Closed Bug 1201229 Opened 9 years ago Closed 9 years ago

nsIHttpChannel::GetRequest/ResponseHeader is a footgun

Categories

(Core :: Networking: HTTP, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

Details

Attachments

(1 file)

See bug 1200869 as an example of the problem.  These APIs don't touch the out string argument if the header doesn't exist, and instead throw an exception.  This makes many C++ callers potentially unsafe since they may end up using whatever was in the string before the call.

Jason, do you think we can/should fix this?
Flags: needinfo?(jduell.mcbugs)
I can't see 1180091 (cc me?)

This behavior is exposed to JS currently, so I'm guessing we should be careful about breaking addsons.  Would you be happier with an alternate API that writes a blank string into the argument?  (Note that this then makes it ambiguous whether a header was present and blank-valued, or missing entirely).
Flags: needinfo?(jduell.mcbugs)
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #1)
> I can't see 1180091 (cc me?)

Looks like someone beat me to it!  :-)

> This behavior is exposed to JS currently, so I'm guessing we should be
> careful about breaking addsons.

Is it though?  nsHttpHeaderArray returns NS_ERROR_NOT_AVAILABLE which will propagate up to these functions, and then XPConnect converts that error into an exception, so JS never gets to see the return value.  It is only internal C++ callers that we need to care about.  My question was mostly regarding those callers.
I'm testing a patch on try to see what happens!
This fixes nsIHttpChannel::GetRequestHeader() and
nsIHttpChannel::GetResponseHeader() to always empty out their string
argument even when they fail.  This prevents programming mistakes of
passing the same string object to multiple of these calls and using the
string value without checking the nsresult error code, since otherwise
the string value may be unchanged from a previous call.

Note that this doesn't affect JS consumers of these APIs since we only
empty out the string argument in case the method fails, which will be
translated to a JS exception, and the JS code will never get to see the
emptied string.
Attachment #8656550 - Flags: review?(jduell.mcbugs)
Hmm, Liz, why do you think this should be tracked for 43?  I don't think this is a serious issue, it's more about eliminating a footgun in our code for the future.
Flags: needinfo?(lhenry)
Ehsan: I was following up from another bug and thought this sounded like something that people may want to uplift (if it gets fixed). I'll just untrack it since you think it's not important for relman to follow up. Thanks for the feedback!
Flags: needinfo?(lhenry)
I don't think we'd want to uplift this.  Thanks for checking!
Comment on attachment 8656550 [details] [diff] [review]
Return an empty string for a header when an error occurs

Review of attachment 8656550 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Jason asked me to review this bug. I thing that his comment in this bug was actually meant for bug 815299.
Attachment #8656550 - Flags: review?(jduell.mcbugs) → review+
(In reply to Dragana Damjanovic [:dragana] from comment #9)
> Jason asked me to review this bug. I thing that his comment in this bug was
> actually meant for bug 815299.

Hmm, no, why do you think that?
(In reply to Ehsan Akhgari (don't ask for review please) from comment #10)
> (In reply to Dragana Damjanovic [:dragana] from comment #9)
> > Jason asked me to review this bug. I thing that his comment in this bug was
> > actually meant for bug 815299.
> 
> Hmm, no, why do you think that?

I am probably wrong.
My thoughts: Because actually being able to write empty string into request header could break addons if they always expect to get a value if the call to GetRequestHeader does not throw. Also both patches independently could potentially break addons, hopefully they will not.
Oh, I see what you mean now.  Yes, that bug can break add-ons at least in theory, but this one cannot (please see comment 2.)
https://hg.mozilla.org/mozilla-central/rev/cd8cf8418f05
Assignee: nobody → ehsan
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: