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

formatting issues with embedded semi-colons #92

Closed
pmackay opened this issue Feb 14, 2016 · 8 comments
Closed

formatting issues with embedded semi-colons #92

pmackay opened this issue Feb 14, 2016 · 8 comments

Comments

@pmackay
Copy link
Contributor

pmackay commented Feb 14, 2016

A site config that included this:

add_header X-Xss-Protection "1; mode=block" always

was making the formatting look odd because of the semi-colon in the middle of the statement. In the replace I tried adding a newline to match on the end of the line, e.g.

v.replace(";\n",";\n ")

which worked but its not a complete fix. Perhaps a regex match to end of line including any preceeding whitespace would work. Do you think this would be a useful fix?

@jdauphant
Copy link
Owner

You have alternative notation that should work in theses cases :
https://github.com/jdauphant/ansible-role-nginx/releases/tag/v1.2

@andrenarchy
Copy link

It's not just a cosmetic problem: we ran into the same issue and our site wasn't reachable at all with firefox because headers with newlines are disallowed in http2 (see here and here).

@jdauphant: Why does the nginx role modify the strings at all? Wouldn't it be better to just remove the manipulations?

@nschloe
Copy link

nschloe commented Feb 17, 2016

+1
Here are more details about the spec: http://stackoverflow.com/a/31324422/353337.

@jdauphant
Copy link
Owner

@andrenarchy Have you see the new notation ? (https://github.com/jdauphant/ansible-role-nginx/releases/tag/v1.2 )
The new notation is not cosmetic, it doesn't edit string at all (except spacing after new line : https://github.com/jdauphant/ansible-role-nginx/blob/master/templates/site.conf.j2#L5).

Old notation are just keeped has compatibility

@nschloe
Copy link

nschloe commented Feb 17, 2016

Old notation are just keeped has compatibility

Considering this bug, I guess removing the old, buggy notation and bumping the major version would be a sensible thing to do.

@andrenarchy
Copy link

@jdauphant Yes, it works with the "new notation". The question is: why should these strings be modified at all? Removing the string modifications wouldn't break compatibility – it would increase compatibility because it doesn't break headers anymore (and probably other settings).

@jdauphant
Copy link
Owner

@nschloe I was thinking about that the time when I introduce the notation.
It's a good idea.
@andrenarchy The string modification are here only to provide readable files, it's debatable some people want other doesn't want that. But if we remove old notation to replace by the new notation, we can provide both.

@jdauphant
Copy link
Owner

If someone want to push a PR that remove old syntax and promote the new syntax, I will be happy to merge :)

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

No branches or pull requests

4 participants