Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ExternalWebSession: avoid extra slashes when the path is "" #3387

Closed

Conversation

zenhack
Copy link
Collaborator

@zenhack zenhack commented Jul 14, 2020

Currently, if the caller makes a request to an ExternalWebSession with
path = "", An extra trailing slash is appended to the URL. This makes
capabilities for non-"directory" URLs useless; e.g. if the
ExternalWebSession points to the URL:

http://example.com/rss.xml

Then when trying to fetch that exact URL (rather than something under
it), the requested URL will most likely be:

http://example.com/rss.xml/

...which will probably result in 404.

This patch fixes the behavior, such that no additional slash is added if
path = "".

Currently, if the caller makes a request to an ExternalWebSession with
path = "", An extra trailing slash is appended to the URL. This makes
capabilities for non-"directory" URLs useless; e.g. if the
ExternalWebSession points to the URL:

    http://example.com/rss.xml

Then when trying to fetch that exact URL (rather than something under
it), the requested URL will most likely be:

    http://example.com/rss.xml/

...which will probably result in 404.

This patch fixes the behavior, such that no additional slash is added if
path = "".
@ocdtrekkie ocdtrekkie added app-platform App/Sandstorm integration features ready-for-review We think this is ready for review labels Jul 14, 2020
@kentonv
Copy link
Member

kentonv commented Aug 1, 2020

Hmm, unfortunately this means there's now no way to specify that you want the trailing slash.

Maybe we could trigger this when the path is null, rather than the empty string?

@zenhack
Copy link
Collaborator Author

zenhack commented Aug 1, 2020

One problem with that is that afaik there's no way for an app using the bridge's proxy functionality for this to actually set the path to null, so it would remain unusable except for apps speaking capnp directly.

I think you could still send the trailing slash if you're willing to put the trailing slash in canonicalUrl when making the powerbox request in the first place? That means you'd need a separate cap if you needed to do both though...

@kentonv
Copy link
Member

kentonv commented Aug 2, 2020

I know this is ugly but I think we need to be explicit somehow. As it stands this change could change behavior of existing apps, and I'm not completely confident saying no one depends on this.

It sucks that HTTP treats /foo and /foo/ differently, ugh...

@ocdtrekkie ocdtrekkie removed the ready-for-review We think this is ready for review label Aug 4, 2020
@zenhack
Copy link
Collaborator Author

zenhack commented Nov 19, 2020

Looking back at this, I'm not sure it makes a ton of sense to try to twist ApiSession into handling this case; the docs already suggest you're supposed to be using this to access oauth endpoints and such, and this use case probably won't come up in that context.

In the original context of this PR, I was trying to request individual URLs as part of the bridge I wrote for ttrss -- but I ended up just switching to requesting the whole domain anyway, since otherwise you got way too many pb requests. So this has lost its immediacy (which is why it's been sitting here for months...)

Perhaps we should just close this, and if we find we want to be able to request individual URLs, we add a separate interface that users can request for that purpose; that way we don't have to worry about backwards compatibility concerns, or shoehorn this exact interface into that use case.

I'm going to go ahead and close, though if anyone has opinions I'm willing to discuss further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
app-platform App/Sandstorm integration features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants