Skip to content
This repository has been archived by the owner on Oct 1, 2018. It is now read-only.

warning: (2) duplicate definition of 'Set-Cookie' header #75

Closed
Lexicality opened this issue Feb 4, 2014 · 7 comments · Fixed by #229
Closed

warning: (2) duplicate definition of 'Set-Cookie' header #75

Lexicality opened this issue Feb 4, 2014 · 7 comments · Fixed by #229

Comments

@Lexicality
Copy link

HeaderParser.h#L154 triggers a warning when multiple headers with the same name are defined, for instance by the code:

+ Headers

    ```
    Set-Cookie: wordpress_1c8dfe62d9b0bf5f58269c78cd28d961=user%7C1389435697%7C79092fdcb0a5924559d132dfaaad1c74; path=/wp-content/plugins; httponly
    Set-Cookie: wordpress_1c8dfe62d9b0bf5f58269c78cd28d961=user%7C1389435697%7C79092fdcb0a5924559d132dfaaad1c74; path=/; httponly
    Set-Cookie: wordpress_logged_in_1c8dfe62d9b0bf5f58269c78cd28d961=user%7C1389435697%7C9ade64138438767e2f437e00cf8441fe; path=/; httponly
    ```

However, the API Blueprint specification doesn't specify this restriction. Is there a reason for it?

@zdne
Copy link
Contributor

zdne commented Feb 10, 2014

@Lexicality

This warning is a safety feature to warn user about possible redefinition of an HTTP header. As per HTTP spec If an HTTP header is defined multiple times it must be OK to concatenate its values.

However I think it would be safe to implement an exception to this for the Set-Cookie HTTP header ONLY.

Scheduled.

@Lexicality
Copy link
Author

Awesome, thanks.

On 10 February 2014 13:01, Z notifications@github.com wrote:

@Lexicality https://github.com/Lexicality

This warning is a safety feature to warn user about possible redefinition
of an HTTP header. As per HTTP spec If an HTTP header is defined multiple
times it must be OK to concatenate its values.

However I think it would be safe to implement an exception to this for the
Set-Cookie HTTP header ONLY.

Scheduled.


Reply to this email directly or view it on GitHubhttps://github.com//issues/75#issuecomment-34628198
.

@chriskilding
Copy link

I just ran into this problem too, but in my case it is for the Link header, and I was using api-mock (which seems to be closely following the behaviour of Snow Crash on this matter) to test drive an API blueprint.

According to the RFC, and also this Stackoverflow question, it is entirely permissible for certain HTTP headers to be defined multiple times, given the RFC's conditions about that header having comma separated values (emphasis below mine):

"Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded"

Link, like Set-Cookie, happens to be such a header. Therefore Snow Crash should be updated to support this on any suitable header; its current behaviour is to only use the last declared 'Link' header, which is not quite right.

@zdne
Copy link
Contributor

zdne commented Sep 15, 2014

@themasterchef I completely agree.

its current behaviour is to only use the last declared 'Link' header, which is not quite right

This seems not be the behavior of Snow Crash but the subsequent tooling (api-mock in your case). I have just checked this blueprint:

# GET /1
+ Response 

    + Headers

        ```
        Set-Cookie: wordpress_1c8dfe62d9b0bf5f58269c78cd28d961=user%7C1389435697%7C79092fdcb0a5924559d132dfaaad1c74; path=/wp-content/plugins; httponly
        Set-Cookie: wordpress_1c8dfe62d9b0bf5f58269c78cd28d961=user%7C1389435697%7C79092fdcb0a5924559d132dfaaad1c74; path=/; httponly
        Set-Cookie: wordpress_logged_in_1c8dfe62d9b0bf5f58269c78cd28d961=user%7C1389435697%7C9ade64138438767e2f437e00cf8441fe; path=/; httponly
        ```    

Which will result in the following AST (serialization):

{
    "_version": "2.0",
    "metadata": [],
    "name": "",
    "description": "",
    "resourceGroups": [
        {
            "name": "",
            "description": "",
            "resources": [
                {
                    "name": "",
                    "description": "",
                    "uriTemplate": "/1",
                    "model": {},
                    "parameters": [],
                    "actions": [
                        {
                            "name": "",
                            "description": "",
                            "method": "GET",
                            "parameters": [],
                            "examples": [
                                {
                                    "name": "",
                                    "description": "",
                                    "requests": [],
                                    "responses": [
                                        {
                                            "name": "200",
                                            "description": "",
                                            "headers": [
                                                {
                                                    "name": "Set-Cookie",
                                                    "value": "wordpress_1c8dfe62d9b0bf5f58269c78cd28d961=user%7C1389435697%7C79092fdcb0a5924559d132dfaaad1c74; path=/wp-content/plugins; httponly"
                                                },
                                                {
                                                    "name": "Set-Cookie",
                                                    "value": "wordpress_1c8dfe62d9b0bf5f58269c78cd28d961=user%7C1389435697%7C79092fdcb0a5924559d132dfaaad1c74; path=/; httponly"
                                                },
                                                {
                                                    "name": "Set-Cookie",
                                                    "value": "wordpress_logged_in_1c8dfe62d9b0bf5f58269c78cd28d961=user%7C1389435697%7C9ade64138438767e2f437e00cf8441fe; path=/; httponly"
                                                }
                                            ],
                                            "body": "",
                                            "schema": ""
                                        }
                                    ]
                                }
                            ]
                        }
                    ]
                }
            ]
        }
    ]
}

As you can see all three headers are there.

The "fix" on Snow Crash side would be only to NOT warn on multiple definitions of the Set-Cookie or Link headers. However it is up to the tool consuming the AST to make sure all of the header values are used. The actual AST is not going to be different with the warning fix. (Read: Check with the api-mock whether it really uses all the values and concatenates them as it should).

@Lexicality
Copy link
Author

iirc when I posted this I did some slight looking into it and the generated yaml output was using the header names as keys - hence the restriction. (No duplicate keys etc) The output you posted is different so I'm guessing it's been refactored into not-a-problem?

@zdne
Copy link
Contributor

zdne commented Sep 17, 2014

the generated yaml output was using the header names as keys

There was no change to this recently (last 6months or so).

@zdne zdne added this to the HTTP Validation milestone Oct 9, 2014
@zdne zdne closed this as completed in #229 Oct 13, 2014
zdne added a commit that referenced this issue Oct 13, 2014
…definitions

* allow multiple definitions for headers "Set-Cookie" and "Link". Fixes #75
@zdne
Copy link
Contributor

zdne commented Oct 13, 2014

@Lexicality @themasterchef We have just finished the change in parser. The new behavior is to NOT warn when you have multiple Set-Cookie or Link headers specified in one response.

Release pending.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants