You are viewing a plain text version of this content. The canonical link for it is here.
Posted to users@camel.apache.org by Bengt Rodehav <be...@rodehav.com> on 2013/05/03 16:35:40 UTC
Sftp bug in Camel 2.11.0
Unfortunately I seem to have found another bug in Camel 2.11.0 regarding
sftp. I noticed it when I removed the "stepwise=false" argument from my
routes thus enabling the default stepwise way of changing directory.
The problematic section begins at line 404 in SftpOperations.java:
*404:* if (getCurrentDirectory().startsWith(path)) {
*405:* // use relative path
*406:* String p = getCurrentDirectory().substring(path.length());
*407:* if (p.length() == 0) {
*408:* return;
*409:* }
*410:* // the first character must be '/' and hence removed
*411:* path =
UP_DIR_PATTERN.matcher(p).replaceAll("/..").substring(1);
*412:* }
The problem does not arise when stepping down into the directory to poll
but when going back to the directory where we started. Assume that the home
directory in the sftp server is "/" and I want to poll the directory "in"
relative to the root. My endpoint would look something like this:
sftp://user@server/in?password=secret
What happens is that the "i" in "in" is removed and Camel attempts to
change to directory "n". Looking at the code above, the following happens:
- The current directory is "/in" since we have previously succeeded in
moving there and polling for files. We are now attempting to go back to
where we started, therefore "path" is "/".
- On line 406, the variable "p" will be set to "in", thus removing the
leading "/".
- On line 411 an incorrect assumption is made: It is assumed that the first
character must be "/" but the leading "/" was removed on line 406. The
result is therefore that "path" is set to "n" instead of "in".
I haven't investigated when this error was introduced. I did however
compare with the corresponding logic in FtpOperations.java. In that file
the section quoted above does not exist at all. It seems to me that the
purpose with this code is to, stepwise, change directory up to ".." - one
directory at a time instead of going straight to "/". It seems very
unnecessary to me but I assume there is a reason for it. Otherwise, the
easiest change would be to just remove the code above and use the same
logic as in FtpOperations.java.
Currently my only workaround is to use "stepwise=false".
/Bengt
Re: Sftp bug in Camel 2.11.0
Posted by Bengt Rodehav <be...@rodehav.com>.
Yes, I remember we had this discussion a couple of years ago. I just have
never encountered such a situation myself and was curious as to exactly
what happens on those ftp servers that require stepwise. I know that
permissions can sometimes play tricks so that you are allowed to access a
specific directory but not the intermediate directories. Or, which is a
common case, on login you endup in the root directory to which you have no
access. You then need to change to your home directory (not stepwise
because you would not be permitted to). The opposite has always seemed like
a corner case to me.
I know you disagree but that's allright since I can always use
stepwise=false.
Is it also the case that you need to go back to the home directory in a
stepwise fashion? If so, then ftp/ftps should be fixed since that does not
happen there.
/Bengt
2013/5/30 Claus Ibsen <cl...@gmail.com>
> Some FTP servers and platforms requires stepwise. And it has been the
> default ever since Camel 1.x.
>
>
> On Thu, May 30, 2013 at 10:23 AM, Bengt Rodehav <be...@rodehav.com> wrote:
> > Hello Aki - sorry for the late reply.
> >
> > Yes, I do realize that the reason for this code is to do a stepwise
> change
> > of directories when going back to the "home" directory. What I don't
> > understand is why this is necessary. It is only done in sftp - not in
> > ftp/ftps.
> >
> > I have never been in any situation where stepwise is needed at all. But,
> > since stepwise by default is true, I assume that these situations exist
> > (but I think the default should be not to use stepwise...). But, why
> would
> > you have to change directories stepwise when going back up only in sftp
> but
> > not in ftp/ftps?
> >
> > Also, I always disconnect after each poll (I've found it much more
> stable)
> > which means that going back up is totally unnecessary since I will
> > re-connect on next poll and therefore start in the "home"directory again.
> >
> > Of course, fixing the logic might be preferrable to removing it if there
> is
> > a need for this kind of logic...
> >
> > BTW, do you know why the stepwise option exists? What scenario requires
> us
> > to change to a directory in a stepwise fashion instead of changing to
> that
> > directory directly?
> >
> > /Bengt
> >
> >
> > 2013/5/27 Aki Yoshida <el...@gmail.com>
> >
> >> Hi Bengt,
> >> the quoted code itself is needed for the normal case to ensure the
> stepwise
> >> operation moves the path upwards from the current directory to where it
> >> started without touching its upper directory which you potentially have
> no
> >> authorization to access.
> >>
> >> but the code does not handle the case when the path is at the root, as
> you
> >> describe. In that case, this code needs to be skipped as it is not
> needed
> >> (because you started out from the root and you have authorization down
> from
> >> the root to the current directory).
> >>
> >> I commented to the ticket.
> >>
> >> regards, aki
> >>
> >>
> >> 2013/5/6 Bengt Rodehav <be...@rodehav.com>
> >>
> >> > JIRA created:
> >> >
> >> > https://issues.apache.org/jira/browse/CAMEL-6335
> >> >
> >> > /Bengt
> >> >
> >> >
> >> > 2013/5/6 Bengt Rodehav <be...@rodehav.com>
> >> >
> >> > > Hello Hadrian,
> >> > >
> >> > > I'm not really interested in the blame issue. I'm really glad that
> >> there
> >> > > are so many competent people developing Camel which I think is a
> great
> >> > > product. It might be interesting, though, to see how long the bug
> has
> >> > been
> >> > > there and what versions are affected.
> >> > >
> >> > > I've also been thinking why it hasn't been picked up by any tests. I
> >> > > realize that it must be very difficult to design tests for
> integration
> >> > > components like sftp. Ideally you would have a number of
> ftp/ftps/sftp
> >> > > servers to perform integration tests against. But I guess that is
> hard
> >> to
> >> > > maintain. I don't think unit tests (with mocking) will catch bugs
> like
> >> > the
> >> > > above. How are ftp/ftps/sftp integrations tested?
> >> > >
> >> > > I have noticed that some sftp servers silently ignore attempts to
> >> change
> >> > > directory to a non-existing directory - you won't get there but you
> >> won't
> >> > > get an error code either. But some will give you an error code (like
> >> the
> >> > > one we are integrating to right now).
> >> > >
> >> > > I will create a JIRA. As for patching I guess the most important
> part
> >> is
> >> > > to determine if it is necessary to change back to the home directory
> >> in a
> >> > > stepwise fashion or not. If not, then I think we can just remove
> that
> >> > code.
> >> > > If it is necessary, then we must instead fix it.
> >> > >
> >> > > /Bengt
> >> > >
> >> > >
> >> > >
> >> > > 2013/5/3 Hadrian Zbarcea <hz...@gmail.com>
> >> > >
> >> > >> Bengt, many thanks for reporting this.
> >> > >>
> >> > >> Either svn or git tells you pretty quickly how that code got there
> >> (git
> >> > >> blame). Do you mind raising a jira for this embarrassing bug? If
> you
> >> > want
> >> > >> to contribute a patch it'd be highly appreciated. That aside, I'll
> >> look
> >> > >> into it in the afternoon. I'll need to check if that piece of code
> was
> >> > >> meant to address another usecase or was an oversight and
> insufficient
> >> > >> testing.
> >> > >>
> >> > >> Cheers,
> >> > >> Hadrian
> >> > >>
> >> > >>
> >> > >>
> >> > >> On 05/03/2013 10:35 AM, Bengt Rodehav wrote:
> >> > >>
> >> > >>> Unfortunately I seem to have found another bug in Camel 2.11.0
> >> > regarding
> >> > >>> sftp. I noticed it when I removed the "stepwise=false" argument
> from
> >> my
> >> > >>> routes thus enabling the default stepwise way of changing
> directory.
> >> > >>>
> >> > >>> The problematic section begins at line 404 in SftpOperations.java:
> >> > >>>
> >> > >>> *404:* if (getCurrentDirectory().**startsWith(path)) {
> >> > >>> *405:* // use relative path
> >> > >>> *406:* String p = getCurrentDirectory().**
> >> > >>> substring(path.length());
> >> > >>> *407:* if (p.length() == 0) {
> >> > >>> *408:* return;
> >> > >>> *409:* }
> >> > >>> *410:* // the first character must be '/' and hence
> >> removed
> >> > >>> *411:* path =
> >> > >>> UP_DIR_PATTERN.matcher(p).**replaceAll("/..").substring(1)**;
> >> > >>> *412:* }
> >> > >>>
> >> > >>>
> >> > >>>
> >> > >>> The problem does not arise when stepping down into the directory
> to
> >> > poll
> >> > >>> but when going back to the directory where we started. Assume that
> >> the
> >> > >>> home
> >> > >>> directory in the sftp server is "/" and I want to poll the
> directory
> >> > "in"
> >> > >>> relative to the root. My endpoint would look something like this:
> >> > >>>
> >> > >>> sftp://user@server/in?**password=secret
> >> > >>>
> >> > >>> What happens is that the "i" in "in" is removed and Camel
> attempts to
> >> > >>> change to directory "n". Looking at the code above, the following
> >> > >>> happens:
> >> > >>>
> >> > >>> - The current directory is "/in" since we have previously
> succeeded
> >> in
> >> > >>> moving there and polling for files. We are now attempting to go
> back
> >> to
> >> > >>> where we started, therefore "path" is "/".
> >> > >>> - On line 406, the variable "p" will be set to "in", thus removing
> >> the
> >> > >>> leading "/".
> >> > >>> - On line 411 an incorrect assumption is made: It is assumed that
> the
> >> > >>> first
> >> > >>> character must be "/" but the leading "/" was removed on line 406.
> >> The
> >> > >>> result is therefore that "path" is set to "n" instead of "in".
> >> > >>>
> >> > >>> I haven't investigated when this error was introduced. I did
> however
> >> > >>> compare with the corresponding logic in FtpOperations.java. In
> that
> >> > file
> >> > >>> the section quoted above does not exist at all. It seems to me
> that
> >> the
> >> > >>> purpose with this code is to, stepwise, change directory up to
> ".." -
> >> > one
> >> > >>> directory at a time instead of going straight to "/". It seems
> very
> >> > >>> unnecessary to me but I assume there is a reason for it.
> Otherwise,
> >> the
> >> > >>> easiest change would be to just remove the code above and use the
> >> same
> >> > >>> logic as in FtpOperations.java.
> >> > >>>
> >> > >>> Currently my only workaround is to use "stepwise=false".
> >> > >>>
> >> > >>> /Bengt
> >> > >>>
> >> > >>>
> >> > >
> >> >
> >>
>
>
>
> --
> Claus Ibsen
> -----------------
> www.camelone.org: The open source integration conference.
>
> Red Hat, Inc.
> FuseSource is now part of Red Hat
> Email: cibsen@redhat.com
> Web: http://fusesource.com
> Twitter: davsclaus
> Blog: http://davsclaus.com
> Author of Camel in Action: http://www.manning.com/ibsen
>
Re: Sftp bug in Camel 2.11.0
Posted by Claus Ibsen <cl...@gmail.com>.
Some FTP servers and platforms requires stepwise. And it has been the
default ever since Camel 1.x.
On Thu, May 30, 2013 at 10:23 AM, Bengt Rodehav <be...@rodehav.com> wrote:
> Hello Aki - sorry for the late reply.
>
> Yes, I do realize that the reason for this code is to do a stepwise change
> of directories when going back to the "home" directory. What I don't
> understand is why this is necessary. It is only done in sftp - not in
> ftp/ftps.
>
> I have never been in any situation where stepwise is needed at all. But,
> since stepwise by default is true, I assume that these situations exist
> (but I think the default should be not to use stepwise...). But, why would
> you have to change directories stepwise when going back up only in sftp but
> not in ftp/ftps?
>
> Also, I always disconnect after each poll (I've found it much more stable)
> which means that going back up is totally unnecessary since I will
> re-connect on next poll and therefore start in the "home"directory again.
>
> Of course, fixing the logic might be preferrable to removing it if there is
> a need for this kind of logic...
>
> BTW, do you know why the stepwise option exists? What scenario requires us
> to change to a directory in a stepwise fashion instead of changing to that
> directory directly?
>
> /Bengt
>
>
> 2013/5/27 Aki Yoshida <el...@gmail.com>
>
>> Hi Bengt,
>> the quoted code itself is needed for the normal case to ensure the stepwise
>> operation moves the path upwards from the current directory to where it
>> started without touching its upper directory which you potentially have no
>> authorization to access.
>>
>> but the code does not handle the case when the path is at the root, as you
>> describe. In that case, this code needs to be skipped as it is not needed
>> (because you started out from the root and you have authorization down from
>> the root to the current directory).
>>
>> I commented to the ticket.
>>
>> regards, aki
>>
>>
>> 2013/5/6 Bengt Rodehav <be...@rodehav.com>
>>
>> > JIRA created:
>> >
>> > https://issues.apache.org/jira/browse/CAMEL-6335
>> >
>> > /Bengt
>> >
>> >
>> > 2013/5/6 Bengt Rodehav <be...@rodehav.com>
>> >
>> > > Hello Hadrian,
>> > >
>> > > I'm not really interested in the blame issue. I'm really glad that
>> there
>> > > are so many competent people developing Camel which I think is a great
>> > > product. It might be interesting, though, to see how long the bug has
>> > been
>> > > there and what versions are affected.
>> > >
>> > > I've also been thinking why it hasn't been picked up by any tests. I
>> > > realize that it must be very difficult to design tests for integration
>> > > components like sftp. Ideally you would have a number of ftp/ftps/sftp
>> > > servers to perform integration tests against. But I guess that is hard
>> to
>> > > maintain. I don't think unit tests (with mocking) will catch bugs like
>> > the
>> > > above. How are ftp/ftps/sftp integrations tested?
>> > >
>> > > I have noticed that some sftp servers silently ignore attempts to
>> change
>> > > directory to a non-existing directory - you won't get there but you
>> won't
>> > > get an error code either. But some will give you an error code (like
>> the
>> > > one we are integrating to right now).
>> > >
>> > > I will create a JIRA. As for patching I guess the most important part
>> is
>> > > to determine if it is necessary to change back to the home directory
>> in a
>> > > stepwise fashion or not. If not, then I think we can just remove that
>> > code.
>> > > If it is necessary, then we must instead fix it.
>> > >
>> > > /Bengt
>> > >
>> > >
>> > >
>> > > 2013/5/3 Hadrian Zbarcea <hz...@gmail.com>
>> > >
>> > >> Bengt, many thanks for reporting this.
>> > >>
>> > >> Either svn or git tells you pretty quickly how that code got there
>> (git
>> > >> blame). Do you mind raising a jira for this embarrassing bug? If you
>> > want
>> > >> to contribute a patch it'd be highly appreciated. That aside, I'll
>> look
>> > >> into it in the afternoon. I'll need to check if that piece of code was
>> > >> meant to address another usecase or was an oversight and insufficient
>> > >> testing.
>> > >>
>> > >> Cheers,
>> > >> Hadrian
>> > >>
>> > >>
>> > >>
>> > >> On 05/03/2013 10:35 AM, Bengt Rodehav wrote:
>> > >>
>> > >>> Unfortunately I seem to have found another bug in Camel 2.11.0
>> > regarding
>> > >>> sftp. I noticed it when I removed the "stepwise=false" argument from
>> my
>> > >>> routes thus enabling the default stepwise way of changing directory.
>> > >>>
>> > >>> The problematic section begins at line 404 in SftpOperations.java:
>> > >>>
>> > >>> *404:* if (getCurrentDirectory().**startsWith(path)) {
>> > >>> *405:* // use relative path
>> > >>> *406:* String p = getCurrentDirectory().**
>> > >>> substring(path.length());
>> > >>> *407:* if (p.length() == 0) {
>> > >>> *408:* return;
>> > >>> *409:* }
>> > >>> *410:* // the first character must be '/' and hence
>> removed
>> > >>> *411:* path =
>> > >>> UP_DIR_PATTERN.matcher(p).**replaceAll("/..").substring(1)**;
>> > >>> *412:* }
>> > >>>
>> > >>>
>> > >>>
>> > >>> The problem does not arise when stepping down into the directory to
>> > poll
>> > >>> but when going back to the directory where we started. Assume that
>> the
>> > >>> home
>> > >>> directory in the sftp server is "/" and I want to poll the directory
>> > "in"
>> > >>> relative to the root. My endpoint would look something like this:
>> > >>>
>> > >>> sftp://user@server/in?**password=secret
>> > >>>
>> > >>> What happens is that the "i" in "in" is removed and Camel attempts to
>> > >>> change to directory "n". Looking at the code above, the following
>> > >>> happens:
>> > >>>
>> > >>> - The current directory is "/in" since we have previously succeeded
>> in
>> > >>> moving there and polling for files. We are now attempting to go back
>> to
>> > >>> where we started, therefore "path" is "/".
>> > >>> - On line 406, the variable "p" will be set to "in", thus removing
>> the
>> > >>> leading "/".
>> > >>> - On line 411 an incorrect assumption is made: It is assumed that the
>> > >>> first
>> > >>> character must be "/" but the leading "/" was removed on line 406.
>> The
>> > >>> result is therefore that "path" is set to "n" instead of "in".
>> > >>>
>> > >>> I haven't investigated when this error was introduced. I did however
>> > >>> compare with the corresponding logic in FtpOperations.java. In that
>> > file
>> > >>> the section quoted above does not exist at all. It seems to me that
>> the
>> > >>> purpose with this code is to, stepwise, change directory up to ".." -
>> > one
>> > >>> directory at a time instead of going straight to "/". It seems very
>> > >>> unnecessary to me but I assume there is a reason for it. Otherwise,
>> the
>> > >>> easiest change would be to just remove the code above and use the
>> same
>> > >>> logic as in FtpOperations.java.
>> > >>>
>> > >>> Currently my only workaround is to use "stepwise=false".
>> > >>>
>> > >>> /Bengt
>> > >>>
>> > >>>
>> > >
>> >
>>
--
Claus Ibsen
-----------------
www.camelone.org: The open source integration conference.
Red Hat, Inc.
FuseSource is now part of Red Hat
Email: cibsen@redhat.com
Web: http://fusesource.com
Twitter: davsclaus
Blog: http://davsclaus.com
Author of Camel in Action: http://www.manning.com/ibsen
Re: Sftp bug in Camel 2.11.0
Posted by Bengt Rodehav <be...@rodehav.com>.
Hello Aki - sorry for the late reply.
Yes, I do realize that the reason for this code is to do a stepwise change
of directories when going back to the "home" directory. What I don't
understand is why this is necessary. It is only done in sftp - not in
ftp/ftps.
I have never been in any situation where stepwise is needed at all. But,
since stepwise by default is true, I assume that these situations exist
(but I think the default should be not to use stepwise...). But, why would
you have to change directories stepwise when going back up only in sftp but
not in ftp/ftps?
Also, I always disconnect after each poll (I've found it much more stable)
which means that going back up is totally unnecessary since I will
re-connect on next poll and therefore start in the "home"directory again.
Of course, fixing the logic might be preferrable to removing it if there is
a need for this kind of logic...
BTW, do you know why the stepwise option exists? What scenario requires us
to change to a directory in a stepwise fashion instead of changing to that
directory directly?
/Bengt
2013/5/27 Aki Yoshida <el...@gmail.com>
> Hi Bengt,
> the quoted code itself is needed for the normal case to ensure the stepwise
> operation moves the path upwards from the current directory to where it
> started without touching its upper directory which you potentially have no
> authorization to access.
>
> but the code does not handle the case when the path is at the root, as you
> describe. In that case, this code needs to be skipped as it is not needed
> (because you started out from the root and you have authorization down from
> the root to the current directory).
>
> I commented to the ticket.
>
> regards, aki
>
>
> 2013/5/6 Bengt Rodehav <be...@rodehav.com>
>
> > JIRA created:
> >
> > https://issues.apache.org/jira/browse/CAMEL-6335
> >
> > /Bengt
> >
> >
> > 2013/5/6 Bengt Rodehav <be...@rodehav.com>
> >
> > > Hello Hadrian,
> > >
> > > I'm not really interested in the blame issue. I'm really glad that
> there
> > > are so many competent people developing Camel which I think is a great
> > > product. It might be interesting, though, to see how long the bug has
> > been
> > > there and what versions are affected.
> > >
> > > I've also been thinking why it hasn't been picked up by any tests. I
> > > realize that it must be very difficult to design tests for integration
> > > components like sftp. Ideally you would have a number of ftp/ftps/sftp
> > > servers to perform integration tests against. But I guess that is hard
> to
> > > maintain. I don't think unit tests (with mocking) will catch bugs like
> > the
> > > above. How are ftp/ftps/sftp integrations tested?
> > >
> > > I have noticed that some sftp servers silently ignore attempts to
> change
> > > directory to a non-existing directory - you won't get there but you
> won't
> > > get an error code either. But some will give you an error code (like
> the
> > > one we are integrating to right now).
> > >
> > > I will create a JIRA. As for patching I guess the most important part
> is
> > > to determine if it is necessary to change back to the home directory
> in a
> > > stepwise fashion or not. If not, then I think we can just remove that
> > code.
> > > If it is necessary, then we must instead fix it.
> > >
> > > /Bengt
> > >
> > >
> > >
> > > 2013/5/3 Hadrian Zbarcea <hz...@gmail.com>
> > >
> > >> Bengt, many thanks for reporting this.
> > >>
> > >> Either svn or git tells you pretty quickly how that code got there
> (git
> > >> blame). Do you mind raising a jira for this embarrassing bug? If you
> > want
> > >> to contribute a patch it'd be highly appreciated. That aside, I'll
> look
> > >> into it in the afternoon. I'll need to check if that piece of code was
> > >> meant to address another usecase or was an oversight and insufficient
> > >> testing.
> > >>
> > >> Cheers,
> > >> Hadrian
> > >>
> > >>
> > >>
> > >> On 05/03/2013 10:35 AM, Bengt Rodehav wrote:
> > >>
> > >>> Unfortunately I seem to have found another bug in Camel 2.11.0
> > regarding
> > >>> sftp. I noticed it when I removed the "stepwise=false" argument from
> my
> > >>> routes thus enabling the default stepwise way of changing directory.
> > >>>
> > >>> The problematic section begins at line 404 in SftpOperations.java:
> > >>>
> > >>> *404:* if (getCurrentDirectory().**startsWith(path)) {
> > >>> *405:* // use relative path
> > >>> *406:* String p = getCurrentDirectory().**
> > >>> substring(path.length());
> > >>> *407:* if (p.length() == 0) {
> > >>> *408:* return;
> > >>> *409:* }
> > >>> *410:* // the first character must be '/' and hence
> removed
> > >>> *411:* path =
> > >>> UP_DIR_PATTERN.matcher(p).**replaceAll("/..").substring(1)**;
> > >>> *412:* }
> > >>>
> > >>>
> > >>>
> > >>> The problem does not arise when stepping down into the directory to
> > poll
> > >>> but when going back to the directory where we started. Assume that
> the
> > >>> home
> > >>> directory in the sftp server is "/" and I want to poll the directory
> > "in"
> > >>> relative to the root. My endpoint would look something like this:
> > >>>
> > >>> sftp://user@server/in?**password=secret
> > >>>
> > >>> What happens is that the "i" in "in" is removed and Camel attempts to
> > >>> change to directory "n". Looking at the code above, the following
> > >>> happens:
> > >>>
> > >>> - The current directory is "/in" since we have previously succeeded
> in
> > >>> moving there and polling for files. We are now attempting to go back
> to
> > >>> where we started, therefore "path" is "/".
> > >>> - On line 406, the variable "p" will be set to "in", thus removing
> the
> > >>> leading "/".
> > >>> - On line 411 an incorrect assumption is made: It is assumed that the
> > >>> first
> > >>> character must be "/" but the leading "/" was removed on line 406.
> The
> > >>> result is therefore that "path" is set to "n" instead of "in".
> > >>>
> > >>> I haven't investigated when this error was introduced. I did however
> > >>> compare with the corresponding logic in FtpOperations.java. In that
> > file
> > >>> the section quoted above does not exist at all. It seems to me that
> the
> > >>> purpose with this code is to, stepwise, change directory up to ".." -
> > one
> > >>> directory at a time instead of going straight to "/". It seems very
> > >>> unnecessary to me but I assume there is a reason for it. Otherwise,
> the
> > >>> easiest change would be to just remove the code above and use the
> same
> > >>> logic as in FtpOperations.java.
> > >>>
> > >>> Currently my only workaround is to use "stepwise=false".
> > >>>
> > >>> /Bengt
> > >>>
> > >>>
> > >
> >
>
Re: Sftp bug in Camel 2.11.0
Posted by Aki Yoshida <el...@gmail.com>.
Hi Bengt,
the quoted code itself is needed for the normal case to ensure the stepwise
operation moves the path upwards from the current directory to where it
started without touching its upper directory which you potentially have no
authorization to access.
but the code does not handle the case when the path is at the root, as you
describe. In that case, this code needs to be skipped as it is not needed
(because you started out from the root and you have authorization down from
the root to the current directory).
I commented to the ticket.
regards, aki
2013/5/6 Bengt Rodehav <be...@rodehav.com>
> JIRA created:
>
> https://issues.apache.org/jira/browse/CAMEL-6335
>
> /Bengt
>
>
> 2013/5/6 Bengt Rodehav <be...@rodehav.com>
>
> > Hello Hadrian,
> >
> > I'm not really interested in the blame issue. I'm really glad that there
> > are so many competent people developing Camel which I think is a great
> > product. It might be interesting, though, to see how long the bug has
> been
> > there and what versions are affected.
> >
> > I've also been thinking why it hasn't been picked up by any tests. I
> > realize that it must be very difficult to design tests for integration
> > components like sftp. Ideally you would have a number of ftp/ftps/sftp
> > servers to perform integration tests against. But I guess that is hard to
> > maintain. I don't think unit tests (with mocking) will catch bugs like
> the
> > above. How are ftp/ftps/sftp integrations tested?
> >
> > I have noticed that some sftp servers silently ignore attempts to change
> > directory to a non-existing directory - you won't get there but you won't
> > get an error code either. But some will give you an error code (like the
> > one we are integrating to right now).
> >
> > I will create a JIRA. As for patching I guess the most important part is
> > to determine if it is necessary to change back to the home directory in a
> > stepwise fashion or not. If not, then I think we can just remove that
> code.
> > If it is necessary, then we must instead fix it.
> >
> > /Bengt
> >
> >
> >
> > 2013/5/3 Hadrian Zbarcea <hz...@gmail.com>
> >
> >> Bengt, many thanks for reporting this.
> >>
> >> Either svn or git tells you pretty quickly how that code got there (git
> >> blame). Do you mind raising a jira for this embarrassing bug? If you
> want
> >> to contribute a patch it'd be highly appreciated. That aside, I'll look
> >> into it in the afternoon. I'll need to check if that piece of code was
> >> meant to address another usecase or was an oversight and insufficient
> >> testing.
> >>
> >> Cheers,
> >> Hadrian
> >>
> >>
> >>
> >> On 05/03/2013 10:35 AM, Bengt Rodehav wrote:
> >>
> >>> Unfortunately I seem to have found another bug in Camel 2.11.0
> regarding
> >>> sftp. I noticed it when I removed the "stepwise=false" argument from my
> >>> routes thus enabling the default stepwise way of changing directory.
> >>>
> >>> The problematic section begins at line 404 in SftpOperations.java:
> >>>
> >>> *404:* if (getCurrentDirectory().**startsWith(path)) {
> >>> *405:* // use relative path
> >>> *406:* String p = getCurrentDirectory().**
> >>> substring(path.length());
> >>> *407:* if (p.length() == 0) {
> >>> *408:* return;
> >>> *409:* }
> >>> *410:* // the first character must be '/' and hence removed
> >>> *411:* path =
> >>> UP_DIR_PATTERN.matcher(p).**replaceAll("/..").substring(1)**;
> >>> *412:* }
> >>>
> >>>
> >>>
> >>> The problem does not arise when stepping down into the directory to
> poll
> >>> but when going back to the directory where we started. Assume that the
> >>> home
> >>> directory in the sftp server is "/" and I want to poll the directory
> "in"
> >>> relative to the root. My endpoint would look something like this:
> >>>
> >>> sftp://user@server/in?**password=secret
> >>>
> >>> What happens is that the "i" in "in" is removed and Camel attempts to
> >>> change to directory "n". Looking at the code above, the following
> >>> happens:
> >>>
> >>> - The current directory is "/in" since we have previously succeeded in
> >>> moving there and polling for files. We are now attempting to go back to
> >>> where we started, therefore "path" is "/".
> >>> - On line 406, the variable "p" will be set to "in", thus removing the
> >>> leading "/".
> >>> - On line 411 an incorrect assumption is made: It is assumed that the
> >>> first
> >>> character must be "/" but the leading "/" was removed on line 406. The
> >>> result is therefore that "path" is set to "n" instead of "in".
> >>>
> >>> I haven't investigated when this error was introduced. I did however
> >>> compare with the corresponding logic in FtpOperations.java. In that
> file
> >>> the section quoted above does not exist at all. It seems to me that the
> >>> purpose with this code is to, stepwise, change directory up to ".." -
> one
> >>> directory at a time instead of going straight to "/". It seems very
> >>> unnecessary to me but I assume there is a reason for it. Otherwise, the
> >>> easiest change would be to just remove the code above and use the same
> >>> logic as in FtpOperations.java.
> >>>
> >>> Currently my only workaround is to use "stepwise=false".
> >>>
> >>> /Bengt
> >>>
> >>>
> >
>
Re: Sftp bug in Camel 2.11.0
Posted by Bengt Rodehav <be...@rodehav.com>.
JIRA created:
https://issues.apache.org/jira/browse/CAMEL-6335
/Bengt
2013/5/6 Bengt Rodehav <be...@rodehav.com>
> Hello Hadrian,
>
> I'm not really interested in the blame issue. I'm really glad that there
> are so many competent people developing Camel which I think is a great
> product. It might be interesting, though, to see how long the bug has been
> there and what versions are affected.
>
> I've also been thinking why it hasn't been picked up by any tests. I
> realize that it must be very difficult to design tests for integration
> components like sftp. Ideally you would have a number of ftp/ftps/sftp
> servers to perform integration tests against. But I guess that is hard to
> maintain. I don't think unit tests (with mocking) will catch bugs like the
> above. How are ftp/ftps/sftp integrations tested?
>
> I have noticed that some sftp servers silently ignore attempts to change
> directory to a non-existing directory - you won't get there but you won't
> get an error code either. But some will give you an error code (like the
> one we are integrating to right now).
>
> I will create a JIRA. As for patching I guess the most important part is
> to determine if it is necessary to change back to the home directory in a
> stepwise fashion or not. If not, then I think we can just remove that code.
> If it is necessary, then we must instead fix it.
>
> /Bengt
>
>
>
> 2013/5/3 Hadrian Zbarcea <hz...@gmail.com>
>
>> Bengt, many thanks for reporting this.
>>
>> Either svn or git tells you pretty quickly how that code got there (git
>> blame). Do you mind raising a jira for this embarrassing bug? If you want
>> to contribute a patch it'd be highly appreciated. That aside, I'll look
>> into it in the afternoon. I'll need to check if that piece of code was
>> meant to address another usecase or was an oversight and insufficient
>> testing.
>>
>> Cheers,
>> Hadrian
>>
>>
>>
>> On 05/03/2013 10:35 AM, Bengt Rodehav wrote:
>>
>>> Unfortunately I seem to have found another bug in Camel 2.11.0 regarding
>>> sftp. I noticed it when I removed the "stepwise=false" argument from my
>>> routes thus enabling the default stepwise way of changing directory.
>>>
>>> The problematic section begins at line 404 in SftpOperations.java:
>>>
>>> *404:* if (getCurrentDirectory().**startsWith(path)) {
>>> *405:* // use relative path
>>> *406:* String p = getCurrentDirectory().**
>>> substring(path.length());
>>> *407:* if (p.length() == 0) {
>>> *408:* return;
>>> *409:* }
>>> *410:* // the first character must be '/' and hence removed
>>> *411:* path =
>>> UP_DIR_PATTERN.matcher(p).**replaceAll("/..").substring(1)**;
>>> *412:* }
>>>
>>>
>>>
>>> The problem does not arise when stepping down into the directory to poll
>>> but when going back to the directory where we started. Assume that the
>>> home
>>> directory in the sftp server is "/" and I want to poll the directory "in"
>>> relative to the root. My endpoint would look something like this:
>>>
>>> sftp://user@server/in?**password=secret
>>>
>>> What happens is that the "i" in "in" is removed and Camel attempts to
>>> change to directory "n". Looking at the code above, the following
>>> happens:
>>>
>>> - The current directory is "/in" since we have previously succeeded in
>>> moving there and polling for files. We are now attempting to go back to
>>> where we started, therefore "path" is "/".
>>> - On line 406, the variable "p" will be set to "in", thus removing the
>>> leading "/".
>>> - On line 411 an incorrect assumption is made: It is assumed that the
>>> first
>>> character must be "/" but the leading "/" was removed on line 406. The
>>> result is therefore that "path" is set to "n" instead of "in".
>>>
>>> I haven't investigated when this error was introduced. I did however
>>> compare with the corresponding logic in FtpOperations.java. In that file
>>> the section quoted above does not exist at all. It seems to me that the
>>> purpose with this code is to, stepwise, change directory up to ".." - one
>>> directory at a time instead of going straight to "/". It seems very
>>> unnecessary to me but I assume there is a reason for it. Otherwise, the
>>> easiest change would be to just remove the code above and use the same
>>> logic as in FtpOperations.java.
>>>
>>> Currently my only workaround is to use "stepwise=false".
>>>
>>> /Bengt
>>>
>>>
>
Re: Sftp bug in Camel 2.11.0
Posted by Bengt Rodehav <be...@rodehav.com>.
Hello Hadrian,
I'm not really interested in the blame issue. I'm really glad that there
are so many competent people developing Camel which I think is a great
product. It might be interesting, though, to see how long the bug has been
there and what versions are affected.
I've also been thinking why it hasn't been picked up by any tests. I
realize that it must be very difficult to design tests for integration
components like sftp. Ideally you would have a number of ftp/ftps/sftp
servers to perform integration tests against. But I guess that is hard to
maintain. I don't think unit tests (with mocking) will catch bugs like the
above. How are ftp/ftps/sftp integrations tested?
I have noticed that some sftp servers silently ignore attempts to change
directory to a non-existing directory - you won't get there but you won't
get an error code either. But some will give you an error code (like the
one we are integrating to right now).
I will create a JIRA. As for patching I guess the most important part is to
determine if it is necessary to change back to the home directory in a
stepwise fashion or not. If not, then I think we can just remove that code.
If it is necessary, then we must instead fix it.
/Bengt
2013/5/3 Hadrian Zbarcea <hz...@gmail.com>
> Bengt, many thanks for reporting this.
>
> Either svn or git tells you pretty quickly how that code got there (git
> blame). Do you mind raising a jira for this embarrassing bug? If you want
> to contribute a patch it'd be highly appreciated. That aside, I'll look
> into it in the afternoon. I'll need to check if that piece of code was
> meant to address another usecase or was an oversight and insufficient
> testing.
>
> Cheers,
> Hadrian
>
>
>
> On 05/03/2013 10:35 AM, Bengt Rodehav wrote:
>
>> Unfortunately I seem to have found another bug in Camel 2.11.0 regarding
>> sftp. I noticed it when I removed the "stepwise=false" argument from my
>> routes thus enabling the default stepwise way of changing directory.
>>
>> The problematic section begins at line 404 in SftpOperations.java:
>>
>> *404:* if (getCurrentDirectory().**startsWith(path)) {
>> *405:* // use relative path
>> *406:* String p = getCurrentDirectory().**
>> substring(path.length());
>> *407:* if (p.length() == 0) {
>> *408:* return;
>> *409:* }
>> *410:* // the first character must be '/' and hence removed
>> *411:* path =
>> UP_DIR_PATTERN.matcher(p).**replaceAll("/..").substring(1)**;
>> *412:* }
>>
>>
>>
>> The problem does not arise when stepping down into the directory to poll
>> but when going back to the directory where we started. Assume that the
>> home
>> directory in the sftp server is "/" and I want to poll the directory "in"
>> relative to the root. My endpoint would look something like this:
>>
>> sftp://user@server/in?**password=secret
>>
>> What happens is that the "i" in "in" is removed and Camel attempts to
>> change to directory "n". Looking at the code above, the following happens:
>>
>> - The current directory is "/in" since we have previously succeeded in
>> moving there and polling for files. We are now attempting to go back to
>> where we started, therefore "path" is "/".
>> - On line 406, the variable "p" will be set to "in", thus removing the
>> leading "/".
>> - On line 411 an incorrect assumption is made: It is assumed that the
>> first
>> character must be "/" but the leading "/" was removed on line 406. The
>> result is therefore that "path" is set to "n" instead of "in".
>>
>> I haven't investigated when this error was introduced. I did however
>> compare with the corresponding logic in FtpOperations.java. In that file
>> the section quoted above does not exist at all. It seems to me that the
>> purpose with this code is to, stepwise, change directory up to ".." - one
>> directory at a time instead of going straight to "/". It seems very
>> unnecessary to me but I assume there is a reason for it. Otherwise, the
>> easiest change would be to just remove the code above and use the same
>> logic as in FtpOperations.java.
>>
>> Currently my only workaround is to use "stepwise=false".
>>
>> /Bengt
>>
>>
Re: Sftp bug in Camel 2.11.0
Posted by Hadrian Zbarcea <hz...@gmail.com>.
Bengt, many thanks for reporting this.
Either svn or git tells you pretty quickly how that code got there (git
blame). Do you mind raising a jira for this embarrassing bug? If you
want to contribute a patch it'd be highly appreciated. That aside, I'll
look into it in the afternoon. I'll need to check if that piece of code
was meant to address another usecase or was an oversight and
insufficient testing.
Cheers,
Hadrian
On 05/03/2013 10:35 AM, Bengt Rodehav wrote:
> Unfortunately I seem to have found another bug in Camel 2.11.0 regarding
> sftp. I noticed it when I removed the "stepwise=false" argument from my
> routes thus enabling the default stepwise way of changing directory.
>
> The problematic section begins at line 404 in SftpOperations.java:
>
> *404:* if (getCurrentDirectory().startsWith(path)) {
> *405:* // use relative path
> *406:* String p = getCurrentDirectory().substring(path.length());
> *407:* if (p.length() == 0) {
> *408:* return;
> *409:* }
> *410:* // the first character must be '/' and hence removed
> *411:* path =
> UP_DIR_PATTERN.matcher(p).replaceAll("/..").substring(1);
> *412:* }
>
>
> The problem does not arise when stepping down into the directory to poll
> but when going back to the directory where we started. Assume that the home
> directory in the sftp server is "/" and I want to poll the directory "in"
> relative to the root. My endpoint would look something like this:
>
> sftp://user@server/in?password=secret
>
> What happens is that the "i" in "in" is removed and Camel attempts to
> change to directory "n". Looking at the code above, the following happens:
>
> - The current directory is "/in" since we have previously succeeded in
> moving there and polling for files. We are now attempting to go back to
> where we started, therefore "path" is "/".
> - On line 406, the variable "p" will be set to "in", thus removing the
> leading "/".
> - On line 411 an incorrect assumption is made: It is assumed that the first
> character must be "/" but the leading "/" was removed on line 406. The
> result is therefore that "path" is set to "n" instead of "in".
>
> I haven't investigated when this error was introduced. I did however
> compare with the corresponding logic in FtpOperations.java. In that file
> the section quoted above does not exist at all. It seems to me that the
> purpose with this code is to, stepwise, change directory up to ".." - one
> directory at a time instead of going straight to "/". It seems very
> unnecessary to me but I assume there is a reason for it. Otherwise, the
> easiest change would be to just remove the code above and use the same
> logic as in FtpOperations.java.
>
> Currently my only workaround is to use "stepwise=false".
>
> /Bengt
>