Closed Bug 181938 Opened 22 years ago Closed 19 years ago

redirection should be something any protocol can optionally support

Categories

(Core :: Networking, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: darin.moz, Assigned: Biesinger)

References

Details

(Keywords: arch, embed)

Attachments

(3 files, 2 obsolete files)

there's no reason for redirection to be a HTTP specific thing.  we should allow
any protocol handler to redirect a channel to another protocol.

this simply involves moving the onRedirect method from nsIHttpEventSink to some
more generic interface.  the resulting interface might be a good interface to
consider freezing.
Severity: normal → minor
Status: NEW → ASSIGNED
Priority: -- → P5
Target Milestone: --- → mozilla1.3beta
Severity: minor → enhancement
Target Milestone: mozilla1.3beta → Future
-> me

I'll basically move onRedirect from nsIHttpEventSink to nsIRedirectObserver and
change the argument to nsIChannel (rather than nsIHttpChannel).

The question is.. Should it instead have a more generic nsIProtocolEventSink in
case we want other kinds of notifications? I guess if we want to freeze it
sometime soon, then it won't matter anyway.
Assignee: darin → cbiesinger
Status: ASSIGNED → NEW
Priority: P5 → --
Target Milestone: Future → mozilla1.8beta2
how about nsIChannelEventSink?  and maybe we should continue to support
nsIHttpEventSink for a while longer.
nsIChannelEventSink::onChannelRedirect

having "channel" in the method name helps when a class is implementing a dozen
or so interfaces.

also, we should freeze this for 1.8.  i'm fine with it only having this one
method unless someone can think of other events that might be useful.  we should
also freeze nsIProgressEventSink :)
Attached patch patch (obsolete) — Splinter Review
Attachment #176093 - Flags: review?(darin)
Status: NEW → ASSIGNED
So, it just occured to me this morning that we may want to add a flags parameter
to onChannelRedirect that would indicate whether this is a temporary redirect
(302, etc.) or a permanent redirect (301).
Comment on attachment 176093 [details] [diff] [review]
patch

It's important to document that throwing an exception from onChannelRedirect
aborts the redirect.

r=darin otherwise.  if you want to just post a diffdiff for the revisions
that'd be great.
Attachment #176093 - Flags: review?(darin) → review+
Attached patch patch v2 (obsolete) — Splinter Review
great idea about adding the type of the redirect. I'm using a "flags" argument
in this patch; this is probably a bit more extensible, but is it needed? Maybe
a "type" argument would be easier to use.

that failure of this method cancelled the load was mentioned in the last patch;
I made that a bit clearer.
Attachment #176093 - Attachment is obsolete: true
Attachment #176193 - Flags: review?(darin)
Even HTTP can do redirects back to the same URI.  That happens when for example
a site wanted a cookie to be set before proceeding.  That scenario is what leads
to that dialog about infinite redirect looping with a warning to the user that
it may be due to their cookie blocking prefs.

I would also prefer to see an explicit constant for REDIRECT_TEMPORARY (=0). 
Why flag internal redirects?  Why not lump those with temporary redirects?  I'm
curious if you had something specific in mind.  Or, is it just a matter of
adding that because we can?
Comment on attachment 176193 [details] [diff] [review]
patch v2

minusing in favor of my proposed changes
Attachment #176193 - Flags: review?(darin) → review-
> Even HTTP can do redirects back to the same URI.

oh, true, that's a good point.

> I would also prefer to see an explicit constant for REDIRECT_TEMPORARY (=0). 

Well, I made this a flag (to be tested with &, not ==); so I didn't think a
constant for TEMPORARY made sense. that said. changing this to just an enum
would be fine with me - I'm not sure if a flag actually buys us anything.

> Why flag internal redirects?

I was thinking that implementors might want to differentiate between
server-initiated redirects and those that are just implementation details. I did
not think of a specific use case. should I remove it?
We already have a convention for zero-valued constants being defined for values
that are a bit-field.  See nsIRequest::LOAD_NORMAL.

Ultimately, I'm hoping that we can tie the redirection type into session
history, to affect URL bar rewriting.  It might be useful to leverage that
mechanism even in cases where there aren't server initiated redirects but rather
implementation details.  It'll give us more flexibility.  So, that argues for
either getting rid of the internal flag or allowing that bit to be set in
conjunction with temporary or permanent.  Maybe temporary should just have its
own bit.  And, let a value of 0 have no meaning.
well, the interface in this patch doesn't prohibit setting both
REDIRECT_INTERNAL and REDIRECT_PERMANENT... the docs do assume that internal
redirects only change the channel instance, not its URI. maybe that's a bad
assumption.

Hm... a real flag for TEMPORARY would probably make code clearer which tests for
that condition.

> Ultimately, I'm hoping that we can tie the redirection type into session
> history, to affect URL bar rewriting. 

so temporary redirects would not affect the displayed URL? that might have
security implications... (displayed url is not what actually got displayed)
Attached patch patch v3Splinter Review
- add a define for REDIRECT_TEMPORARY (defined to 1)
- change REDIRECT_PERMANENT to 2
- document that one of them will always be set

- let REDIRECT_INTERNAL change the URI if desired
Attachment #176193 - Attachment is obsolete: true
Attachment #176523 - Flags: review?(darin)
(In reply to comment #13)
> well, the interface in this patch doesn't prohibit setting both
> REDIRECT_INTERNAL and REDIRECT_PERMANENT... the docs do assume that internal
> redirects only change the channel instance, not its URI. maybe that's a bad
> assumption.

Yeah, I don't see this being all that valuable.  Someone could always check the
URIs if needed.

 
> Hm... a real flag for TEMPORARY would probably make code clearer which tests for
> that condition.

Agreed.


> > Ultimately, I'm hoping that we can tie the redirection type into session
> > history, to affect URL bar rewriting. 
> 
> so temporary redirects would not affect the displayed URL? that might have
> security implications... (displayed url is not what actually got displayed)

The idea is that the site you think you are going to (the one displayed in the
URL) is the one controlling the redirect.  So if you trust the site you think
you're at then you trust whereever it sends you.  Afterall, the site could
achieve the same result using a <frameset>.  So, I think it would be better to
fix the long standing bug that browser history systems should not reflect the
result of a temporary redirect.  I think there is a bug on this somewhere.  For
example, have you ever noticed that navigating to www.hotmail.com results in
some weird hostname showing up in the URL bar?  That's because of this problem.
 It'd be nicer for users if it still said www.hotmail.com :-)
Comment on attachment 176523 [details] [diff] [review]
patch v3

r=darin
Attachment #176523 - Flags: review?(darin) → review+
Attachment #176523 - Flags: superreview?(bzbarsky)
Comment on attachment 176523 [details] [diff] [review]
patch v3

sr=bzbarsky.

Should we change chrome/jar channels to fire such "internal" redirects when
they open the "real" channel?
Attachment #176523 - Flags: superreview?(bzbarsky) → superreview+
checked in.

Do they change their URI when they do that? The instance stays the same, right?
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
> Do they change their URI when they do that?

Absolutely.  I meant chrome/resource channels.  And they change the URI to a jar
URI.  But yes, the channel instance does not change....  The resource: protocol
handler just resolves the URI and then calls newChannel() on that URI.  chrome:
does the same, unless it's a cached chrome load; then it uses a custom channel
impl that doesn't actually implement nsIChannel right... ;)  Perhaps this code
should in fact implement custom channels?  More on this below.

To be more clear, I want to eliminate the code at
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.cpp#704
(which basically special-cases some schemes so that we don't get the
post-redirect URI for them).  It seems to me this could be addressed by
listening for redirects in the URILoader and then passing whatever data we
garnered from that listener to DoContent (or its replacement on
nsIURIContentListener2?).   I guess I should file a new bug on this, eh?  ;)
This might be a bigger change then we want to do, or it might be a bad idea for
some other reason, but i've always thought that our current redirect-blocking is
a bit reversed. Wouldn't it be better to block redirection by default, and allow
some way (through nsIChannelEventSink or otherwise) to turn it on?

That way we don't have to constantly be on the lookout for security holes...
I filed Bug 285192 on sending onChannelRedirect for chrome/resource
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: