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

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Henry Chang [:hchang]]

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.

Comment 1

[Henry Chang [:hchang]]

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!

Comment 2

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 3

[Henry Chang [:hchang]]

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

Comment 4

[Valentin Gosu [:valentin] (he/him)]

Oh, I understand. Yes, that sounds great!
It's definitely a feature I'd like nsMultiMixedConv to have.

Comment 5

[Henry Chang [:hchang]]

Attachment: Bug1185439.patch

Comment 6

[Henry Chang [:hchang]]

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 :)

Comment 7

[Valentin Gosu [:valentin] (he/him)]

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!

Comment 8

[Henry Chang [:hchang]]

(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.

Comment 9

[Henry Chang [:hchang]]

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?

Comment 10

[Valentin Gosu [:valentin] (he/him)]

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.

Comment 11

[Henry Chang [:hchang]]

(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!

Comment 12

[Henry Chang [:hchang]]

Attachment: Bug1185439.patch

Comment 13

[Henry Chang [:hchang]]

Attachment: Bug1185439.patch

Comment 14

[Henry Chang [:hchang]]

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!

Comment 15

[Valentin Gosu [:valentin] (he/him)]

(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.

Comment 16

[Henry Chang [:hchang]]

(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!

Comment 17

[Valentin Gosu [:valentin] (he/him)]

(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?

Comment 18

[Henry Chang [:hchang]]

(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.

Comment 19

[Valentin Gosu [:valentin] (he/him)]

> 
> 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 :) ]

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 21

[Henry Chang [:hchang]]

(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 :)

Comment 22

[Honza Bambas (:mayhemer)]

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.

Comment 23

[Henry Chang [: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?

Comment 24

[Honza Bambas (:mayhemer)]

(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!

Comment 25

[Henry Chang [:hchang]]

Attachment: Bug1185439.patch

Comment 26

[Henry Chang [:hchang]]

Attachment: Bug1185439.patch

Comment 27

[Henry Chang [:hchang]]

Hi Honza,

Sorry I thought I have flagged you for reviewing this patch :( Could you please have it a review? Thanks!

Comment 28

[Honza Bambas (:mayhemer)]

Ask Valentin or Jason to find someone else.  I'm busy right now.

Comment 29

[Valentin Gosu [:valentin] (he/him)]

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+

Comment 30

[Honza Bambas (:mayhemer)]

Thanks Valentin!

Comment 31

[Henry Chang [:hchang]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #30)
> Thanks Valentin!

Thanks Valentin, too!

Comment 32

[Henry Chang [:hchang]]

Attachment: Bug1185439 (carry r+, changed reviewer)

Comment 33

[Henry Chang [:hchang]]

Attachment: Bug1185439 (carry r+, changed reviewer)

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Please provide a link to a recent Try run.

Comment 35

[Henry Chang [:hchang]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2033141a08c

Sorry for not providing it before :(

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f91ceb68296b

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f91ceb68296b