Closed Bug 1185439 Opened 5 years ago Closed 5 years ago

Packaged apps needs to know the header of the multipart'ed content

Categories

(Core :: Networking, defect, P1)

defect

Tracking

()

RESOLVED FIXED
FxOS-S5 (21Aug)
blocking-b2g 2.5+
Tracking Status
firefox43 --- fixed
b2g-master --- fixed

People

(Reporter: hchang, Assigned: hchang)

References

Details

Attachments

(1 file, 6 obsolete files)

We seem to need to embed headers to the packaged web app for new security model.
The current parsing implementation for packaged web app makes use of
existing multipart parser so additional function/attribute should be required 
to be able to know the content prior to the first sub-resource.
Valentin,

I'd like to put the header as an attribute to nsIMultiPartChannel so that
the client could get the header for later use. Can I have your comment on
this approach? Thanks!
Flags: needinfo?(valentin.gosu)
Assignee: nobody → hchang
Blocks: nsec-verify
Hi Henry,
Do you mean moving the methods from nsIResponseHeadProvider to nsIMultiPartChannel?
I guess that would be ok. If you meant something else, please explain.
Flags: needinfo?(valentin.gosu)
Hi Valentin,

Sorry for confusing.

As proposed by [1], some lines may be added as the package header (not the HTTP response header).
What I intended is to add something like "header" or "preamble" to nsIMultiPartChannel
so that PackagedAppService could obtain the content prior to the first appeared resource.
Since nsIMultiPartChannel already contains some meta info for the content such as "partID"
or "isLastPart", it should make sense to provide other meta info for the client.

Thanks!

[1] https://wiki.mozilla.org/User:Ptheriault/Packagedprivilegedcontent#Privileged_Packages
Oh, I understand. Yes, that sounds great!
It's definitely a feature I'd like nsMultiMixedConv to have.
Attached patch Bug1185439.patch (obsolete) — Splinter Review
Comment on attachment 8636008 [details] [diff] [review]
Bug1185439.patch

Hi Valentin,

The package demonstrate my idea of this feature. It hasn't been tested yet
but I'd like to know your comment in the very early stage. Could you please
have it a quick look? Thanks :)
Attachment #8636008 - Flags: feedback?(valentin.gosu)
Comment on attachment 8636008 [details] [diff] [review]
Bug1185439.patch

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

At first sight the patch looks good.
Please add xpcshell-tests in netwerk/test/unit to make sure everything works fine, and that we handle error cases properly.
Thanks!
Attachment #8636008 - Flags: feedback?(valentin.gosu) → feedback+
(In reply to Valentin Gosu [:valentin] from comment #7)
> Comment on attachment 8636008 [details] [diff] [review]
> Bug1185439.patch
> 
> Review of attachment 8636008 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> At first sight the patch looks good.
> Please add xpcshell-tests in netwerk/test/unit to make sure everything works
> fine, and that we handle error cases properly.
> Thanks!

Cool! Thanks.

I kind of think we might need a nsIPackagePartChannel extended from nsIMultiPartChannel
in the long run for more and more customized features used by package app only.
But for now I think the semantic of 'preamble' is general enough to add to nsIMultiPartChannel.
I'll keep this in mind and be waiting for the good time to put up for discussion.
I am figuring out how to add a test case for this feature. From 'nsIPackagedAppService.idl'
perspective, there seems no API exposed to get the header of the package. 
(the package header info can be only found in nsIChannel and it's not exposed at all)

Since the use of package header might be limited to PackagedAppService.cpp (I suppose) 
so that I didn't expose it to nsIPackagedAppService. To make this feature testable, 
I need to either: 

1) Extend nsIPackagedAppService like appending new callbacks to 
   nsIPackagedAppService.requestURL() 

   or
 
2) Add the package header as the meta data in the cache file (maybe each or package only). 

Valentin, do you have any thought on it and which one do you prefer?
Flags: needinfo?(valentin.gosu)
As I see it, the preamble is something that should only be used inside the packaged app service itself.
We can't really use it to override headers, as there are several security implications of this, and I don't know if appending the preamble to the cache file metadata is actually useful.

My suggestion for testing this feature is to add it to test_multipart_streamconv_application_package.js which tests nsMultiMixedConv. There you have direct access to the multipart channel, and you can verify that the value of channel.preamble is the correct one.

Once we actually use the preamble in the packaged app service, we can add unit tests for those code paths as well.
Flags: needinfo?(valentin.gosu)
(In reply to Valentin Gosu [:valentin] from comment #10)
> As I see it, the preamble is something that should only be used inside the
> packaged app service itself.
> We can't really use it to override headers, as there are several security
> implications of this, and I don't know if appending the preamble to the
> cache file metadata is actually useful.
> 
> My suggestion for testing this feature is to add it to
> test_multipart_streamconv_application_package.js which tests
> nsMultiMixedConv. There you have direct access to the multipart channel, and
> you can verify that the value of channel.preamble is the correct one.
> 
> Once we actually use the preamble in the packaged app service, we can add
> unit tests for those code paths as well.

test_multipart_streamconv_application_package.js seems a good test bed for
this feature. Thanks!
Attached patch Bug1185439.patch (obsolete) — Splinter Review
Attachment #8636008 - Attachment is obsolete: true
Attached patch Bug1185439.patch (obsolete) — Splinter Review
Attachment #8640276 - Attachment is obsolete: true
Comment on attachment 8640277 [details] [diff] [review]
Bug1185439.patch

Hi Valentin,

I added the test case to test_multipart_streamconv_application_package.
A minor requirement I didn't mention earlier is that the 'package header' 
requires 'boundary' to be defined in the HTTP header otherwise we are not
able to parse it. 

Consider the current relaxed constraint of packaged app, the parsing rule
for a packaged app becomes the following:

if (package starts with '--') {
  // If 'boundary' was set in the HTTP header, make sure they matches.
  // Extract the boundary and prepare to parse the first part.
} else if ('boundary' was NOT set in the HTTP header) {
  // Return error
} else {
  // Put the data to the preamble until we see the boundary
}

Can I have your thought to this? Thanks!
Attachment #8640277 - Flags: feedback?(valentin.gosu)
Blocks: 1188717
(In reply to Henry Chang [:henry] from comment #14)
> Comment on attachment 8640277 [details] [diff] [review]
> Bug1185439.patch
> 
> Hi Valentin,
> 
> I added the test case to test_multipart_streamconv_application_package.
> A minor requirement I didn't mention earlier is that the 'package header' 
> requires 'boundary' to be defined in the HTTP header otherwise we are not
> able to parse it. 
> 
> Consider the current relaxed constraint of packaged app, the parsing rule
> for a packaged app becomes the following:
> 

Actually, I think we don't necessarily need to have the boundary in the header.
Looking at RFC1341 and http://www.w3.org/TR/web-packaging/ I think we can probably do without the header boundary, by treating everything in the package before CR LF "--" as a preamble.

So the rule becomes:

> if (package starts with '--') {
>   // If 'boundary' was set in the HTTP header, make sure they matches.
>   // Extract the boundary and prepare to parse the first part.
> } else if ('boundary' was NOT set in the HTTP header) {
>   // Return error

Read content until CR LF "--". Extract the preamble, continue parsing the package

> } else {
>   // Put the data to the preamble until we see the boundary
> }

The RFC doesn't really require a CRLF before the boundary in the package, but it assumes the boundary is in the header anyway. In the web-packaging spec the boundary is separated from the preamble with a CRLF CRLF, so I think we could require that when the boundary is missing from the header.
(In reply to Valentin Gosu [:valentin] from comment #15)
> (In reply to Henry Chang [:henry] from comment #14)
> > Comment on attachment 8640277 [details] [diff] [review]
> > Bug1185439.patch
> > 
> > Hi Valentin,
> > 
> > I added the test case to test_multipart_streamconv_application_package.
> > A minor requirement I didn't mention earlier is that the 'package header' 
> > requires 'boundary' to be defined in the HTTP header otherwise we are not
> > able to parse it. 
> > 
> > Consider the current relaxed constraint of packaged app, the parsing rule
> > for a packaged app becomes the following:
> > 
> 
> Actually, I think we don't necessarily need to have the boundary in the
> header.
> Looking at RFC1341 and http://www.w3.org/TR/web-packaging/ I think we can
> probably do without the header boundary, by treating everything in the
> package before CR LF "--" as a preamble.
> 

But what if the preamble contains CR LF '--'? Having to escape the pattern 
seems weird?

> 
> The RFC doesn't really require a CRLF before the boundary in the package,
> but it assumes the boundary is in the header anyway. In the web-packaging
> spec the boundary is separated from the preamble with a CRLF CRLF, so I
> think we could require that when the boundary is missing from the header.

If escaping "CR LF --" is acceptable, then I can implement it like you suggested.
Thanks!
(In reply to Henry Chang [:henry] from comment #16)
> (In reply to Valentin Gosu [:valentin] from comment #15)
> > (In reply to Henry Chang [:henry] from comment #14)
> > > Comment on attachment 8640277 [details] [diff] [review]
> > > Bug1185439.patch
> > > 
> > > Hi Valentin,
> > > 
> > > I added the test case to test_multipart_streamconv_application_package.
> > > A minor requirement I didn't mention earlier is that the 'package header' 
> > > requires 'boundary' to be defined in the HTTP header otherwise we are not
> > > able to parse it. 
> > > 
> > > Consider the current relaxed constraint of packaged app, the parsing rule
> > > for a packaged app becomes the following:
> > > 
> > 
> > Actually, I think we don't necessarily need to have the boundary in the
> > header.
> > Looking at RFC1341 and http://www.w3.org/TR/web-packaging/ I think we can
> > probably do without the header boundary, by treating everything in the
> > package before CR LF "--" as a preamble.
> > 
> 
> But what if the preamble contains CR LF '--'? Having to escape the pattern 
> seems weird?

That would not be allowed, just as we would not allow "--boundary" in the preamble.
I don't know if we actually need to escape the pattern, just disallow it.

> 
> > 
> > The RFC doesn't really require a CRLF before the boundary in the package,
> > but it assumes the boundary is in the header anyway. In the web-packaging
> > spec the boundary is separated from the preamble with a CRLF CRLF, so I
> > think we could require that when the boundary is missing from the header.
> 
> If escaping "CR LF --" is acceptable, then I can implement it like you
> suggested.

Given that we want to be able to serve packages without a header boundary, I think this is the way to go.
Jonas, do you have any thoughts about this?
Flags: needinfo?(jonas)
(In reply to Valentin Gosu [:valentin] from comment #17)
> (In reply to Henry Chang [:henry] from comment #16)
> > (In reply to Valentin Gosu [:valentin] from comment #15)
> > > (In reply to Henry Chang [:henry] from comment #14)
> > > > Comment on attachment 8640277 [details] [diff] [review]
> > > > Bug1185439.patch
> > > > 
> > > > Hi Valentin,
> > > > 
> > > > I added the test case to test_multipart_streamconv_application_package.
> > > > A minor requirement I didn't mention earlier is that the 'package header' 
> > > > requires 'boundary' to be defined in the HTTP header otherwise we are not
> > > > able to parse it. 
> > > > 
> > > > Consider the current relaxed constraint of packaged app, the parsing rule
> > > > for a packaged app becomes the following:
> > > > 
> > > 
> > > Actually, I think we don't necessarily need to have the boundary in the
> > > header.
> > > Looking at RFC1341 and http://www.w3.org/TR/web-packaging/ I think we can
> > > probably do without the header boundary, by treating everything in the
> > > package before CR LF "--" as a preamble.
> > > 
> > 
> > But what if the preamble contains CR LF '--'? Having to escape the pattern 
> > seems weird?
> 
> That would not be allowed, just as we would not allow "--boundary" in the
> preamble.
> I don't know if we actually need to escape the pattern, just disallow it.
> 

Disallowing "--boundary" is okay since the boundary can be made to never
appear in the content. However, disallowing a constant string (like CR LF --)
seems to restrictive. Do you think so?

> > 
> > > 
> > > The RFC doesn't really require a CRLF before the boundary in the package,
> > > but it assumes the boundary is in the header anyway. In the web-packaging
> > > spec the boundary is separated from the preamble with a CRLF CRLF, so I
> > > think we could require that when the boundary is missing from the header.
> > 
> > If escaping "CR LF --" is acceptable, then I can implement it like you
> > suggested.
> 
> Given that we want to be able to serve packages without a header boundary, I
> think this is the way to go.
> Jonas, do you have any thoughts about this?

Only the signed package requires a header boundary. The web-packaging w3c draft 
(for a unsigned package) still works without the header.
> 
> Disallowing "--boundary" is okay since the boundary can be made to never
> appear in the content. However, disallowing a constant string (like CR LF --)
> seems to restrictive. Do you think so?

> Only the signed package requires a header boundary. The web-packaging w3c
> draft 
> (for a unsigned package) still works without the header.

Fair enough. I guess that might work well enough for now.
This is actually more of a spec corner case, so I guess Jonas should make the call [ assuming Honza approves of the implementation :) ]
I definitely don't want to involve http headers. That feels like it'll make things pretty complex, even if it is optional.

Given that the format of the preamble is something like

(field-name ":" [field-value] CRLF)*

Where both field-name and field-value should not contain CRLF. The only way that you could end up with CRLF "--" in the preamble is if you have a field-name starting with "--". Not allowing that seems ok with me.

In other words, it seems ok to me that the preamble can't contain CRLF "--".

Let me know if I'm missing something?
Flags: needinfo?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #20)
> I definitely don't want to involve http headers. That feels like it'll make
> things pretty complex, even if it is optional.
> 
> Given that the format of the preamble is something like
> 
> (field-name ":" [field-value] CRLF)*
> 
> Where both field-name and field-value should not contain CRLF. The only way
> that you could end up with CRLF "--" in the preamble is if you have a
> field-name starting with "--". Not allowing that seems ok with me.
> 
> In other words, it seems ok to me that the preamble can't contain CRLF "--".
> 
> Let me know if I'm missing something?

If the use of the preamble is limited to 'application/package' content type
then it sounds enough. Let me have this implemented for the new parsing rule.

Thanks :)
Henry, since you want to do changes to the parser code here, I want to sync with you.  

I plan to rewrite it (do a cleanup) with the new mozilla::Tokenizer class, very soon.  

The question here is: is the change you want to introduce just simple and I should wait?  If you want to do any large changes then the Tokenizer should be used for a complete overhaul.

Thanks.
Flags: needinfo?(hchang)
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #22)
> Henry, since you want to do changes to the parser code here, I want to sync
> with you.  
> 
> I plan to rewrite it (do a cleanup) with the new mozilla::Tokenizer class,
> very soon.  
> 
> The question here is: is the change you want to introduce just simple and I
> should wait?  If you want to do any large changes then the Tokenizer should
> be used for a complete overhaul.
> 
> Thanks.

My change would be like:

1) Add a ProbeToken() function to probe the token for the package if it does not start with '--'.
   when first OnDataAvailable (just search for 'CR LF --' and another CR)

2) If the token is not found, reset 'mFirstOnData' flag to collect more data for token probe.

Is it a big change to you?
Flags: needinfo?(hchang)
Flags: needinfo?(honzab.moz)
(In reply to Henry Chang [:henry] from comment #23)
> (In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #22)
> > Henry, since you want to do changes to the parser code here, I want to sync
> > with you.  
> > 
> > I plan to rewrite it (do a cleanup) with the new mozilla::Tokenizer class,
> > very soon.  
> > 
> > The question here is: is the change you want to introduce just simple and I
> > should wait?  If you want to do any large changes then the Tokenizer should
> > be used for a complete overhaul.
> > 
> > Thanks.
> 
> My change would be like:
> 
> 1) Add a ProbeToken() function to probe the token for the package if it does
> not start with '--'.
>    when first OnDataAvailable (just search for 'CR LF --' and another CR)
> 
> 2) If the token is not found, reset 'mFirstOnData' flag to collect more data
> for token probe.
> 
> Is it a big change to you?

That seems simple.  I'll wait then.  Thanks!
Flags: needinfo?(honzab.moz)
Attached patch Bug1185439.patch (obsolete) — Splinter Review
Attachment #8640277 - Attachment is obsolete: true
Attachment #8640277 - Flags: feedback?(valentin.gosu)
Attached patch Bug1185439.patch (obsolete) — Splinter Review
Attachment #8642236 - Attachment is obsolete: true
blocking-b2g: --- → 2.5+
Target Milestone: --- → FxOS-S5 (21Aug)
Hi Honza,

Sorry I thought I have flagged you for reviewing this patch :( Could you please have it a review? Thanks!
Flags: needinfo?(honzab.moz)
Ask Valentin or Jason to find someone else.  I'm busy right now.
Flags: needinfo?(honzab.moz)
Comment on attachment 8642237 [details] [diff] [review]
Bug1185439.patch

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

If Honza is OK with delegating the review to me, then r+
Attachment #8642237 - Flags: review+
Thanks Valentin!
(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #30)
> Thanks Valentin!

Thanks Valentin, too!
Attachment #8642237 - Attachment is obsolete: true
Attachment #8645710 - Attachment is obsolete: true
Attachment #8645715 - Flags: review+
Keywords: checkin-needed
Please provide a link to a recent Try run.
Keywords: checkin-needed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.