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

Adding header to previous empty value results in incorrectly formatted message #53

Open
huw1990 opened this issue Jan 6, 2020 · 5 comments
Assignees

Comments

@huw1990
Copy link

@huw1990 huw1990 commented Jan 6, 2020

If you add a header with a non-empty value to a message containing the same header with an empty value, the result is an incorrectly formatted message. For example, given an existing message of:

...
Supported:
...

Adding a "Supported: timer" header to that message results in the following:

...
Support: ,timer
...

It's a reasonable expectation for the value to be "Supported: timer" after adding the new header, since it's effectively overwriting the initial empty value of "no extensions are supported" as specified in RFC 3261, section 20.37.

@vladimirralev

This comment has been minimized.

Copy link
Collaborator

@vladimirralev vladimirralev commented Jan 6, 2020

Yep I see. Thanks for the report.

vladimirralev added a commit to vladimirralev/jsip that referenced this issue Jan 6, 2020
vladimirralev added a commit to vladimirralev/jsip that referenced this issue Jan 6, 2020
vladimirralev added a commit to vladimirralev/jsip that referenced this issue Jan 6, 2020
@vladimirralev vladimirralev self-assigned this Jan 6, 2020
@huw1990

This comment has been minimized.

Copy link
Author

@huw1990 huw1990 commented Jan 13, 2020

Thanks for the fix. It looks like the AcceptLanguageParserTest is broken as a result of these changes.

@vladimirralev

This comment has been minimized.

Copy link
Collaborator

@vladimirralev vladimirralev commented Jan 13, 2020

Yes, this is a bit of a complicated situation. I am away for a couple of days, will check it out on Wednesday.

@vladimirralev vladimirralev reopened this Jan 13, 2020
@vladimirralev

This comment has been minimized.

Copy link
Collaborator

@vladimirralev vladimirralev commented Jan 14, 2020

Just adding this here for consideration.

The Accept-Language request-header field is similar to Accept, but
restricts the set of natural languages that are preferred as a
response to the request.

      Accept-Language = "Accept-Language" ":"
                        1#( language-range [ ";" "q" "=" qvalue ] )

      language-range  = ( ( 1*8ALPHA *( "-" 1*8ALPHA ) ) | "*" )

The test is technically illegal, but the stack shouldn't enforce legality of this header to support use-cases related to broken clients and creation of torture tools meant to test 3rd parties against reasonably malformed headers.

Combined with the fact that this test is in the TCK which is the spec benchmark we can give it more weight towards the freedom.

I am curious if your use case could get away gracefully by using the setHeader() method. Semantically it may be more consistent to do something like which would overwrite previous headers:

SupportedHeader support = protocolObjects.headerFactory
                    .createSupportedHeader("timer");
request.setHeader(support);
@huw1990

This comment has been minimized.

Copy link
Author

@huw1990 huw1990 commented Jan 14, 2020

Our use case is to add extra Supported header values on top of what was already provided, rather than overwriting and replacing with our own. We could workaround this by querying for this specific case of an empty header value, it just seemed like something that would be better to handle in the stack, since adding a non-empty value to an already empty value is accepted, but produces an invalid message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.