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

[SecurityHeaders] Added a possibility for no follow redirects #6212

Merged
merged 5 commits into from
Feb 27, 2021

Conversation

mbogh
Copy link
Contributor

@mbogh mbogh commented Feb 23, 2021

Added a possibility for no follow redirects on Security Headers badge like:
image

The naming of the parameter is a bit reverse but could not figure out if it is possible introduce a new parameter without breaking existing badges.

fixes #6211

@shields-ci
Copy link

shields-ci commented Feb 23, 2021

Messages
📖 ✨ Thanks for your contribution to Shields, @mbogh!

Generated by 🚫 dangerJS against fc0dd32

@mbogh mbogh changed the title [Security Headers] Added a possibility for no follow redirects [Security-Headers] Added a possibility for no follow redirects Feb 23, 2021
@mbogh mbogh force-pushed the security-headers-no-follow-redirects branch from 336a15b to 016a06d Compare February 23, 2021 10:07
@mbogh mbogh changed the title [Security-Headers] Added a possibility for no follow redirects [SecurityHeaders] Added a possibility for no follow redirects Feb 23, 2021
@mbogh mbogh force-pushed the security-headers-no-follow-redirects branch from 016a06d to 1a112dc Compare February 23, 2021 10:25
@calebcartwright calebcartwright added the service-badge New or updated service badge label Feb 24, 2021
@calebcartwright
Copy link
Member

calebcartwright commented Feb 24, 2021

The naming of the parameter is a bit reverse but could not figure out if it is possible introduce a new parameter without breaking existing badges.

Yeah that does make it feel a bit awkward. I suppose another way would be to just have followRedirects as the badge url query param, with the values confined to true and false and the default set to true. That won't play quite as nice in the badge builder window on the Shields.io site (instead of a checkbox it'd have a text box that would require typing the literal true or false) but it makes for a more natural query param IMO.

cc @badges/shields-maintainers for any other perspectives, I'm a little torn myself

@PyvesB
Copy link
Member

PyvesB commented Feb 24, 2021

I suppose another way would be to just have followRedirects as the badge url query param, with the values confined to true and false and the default set to true. That won't play quite as nice in the badge builder window on the Shields.io site (instead of a checkbox it'd have a text box that would require typing the literal true or false) but it makes for a more natural query param IMO.

Personally I would favour having noFollowRedirects with the simple UI behaviour rather than having followRedirects with true/false. Alternatively, we could word things as ignoreRedirects, at the cost of losing the verb "follow" which is commonly used in the context of HTTP redirects.

@chris48s
Copy link
Member

I think I'm for noFollowRedirects too 👍

In general the way we do boolean params is presence in the qs means true, so maintaining that consistency is useful

Also, the current default is probably the behaviour most users want for this badge so I think it makes more sense if the route most people want is /security-headers not /security-headers?followRedirects=true. I'd think of this as analogous to the JIRA disableStrictSSL param.

@calebcartwright
Copy link
Member

I think it makes more sense if the route most people want is /security-headers

That would still be the case, the absence of the query param would still result in redirects being followed.

I don't feel too strongly about this one though, just something about noFollowRedirects is tough for me 😄. I think I'd be more okay with that if the name was changed to something like ignoreRedirects as @PyvesB suggested, or skipRedirects etc.

@chris48s
Copy link
Member

ignoreRedirects or disableRedirects also works for me 👍

@calebcartwright
Copy link
Member

Sounds like ignoreRedirects is the winner then. @mbogh could you update the param name when you get a chance? The rest LGTM.

Also just out of curiosity, what does a rating of R correlate to? Hopefully this isn't a scale that runs from A through at least R 😄

@mbogh
Copy link
Contributor Author

mbogh commented Feb 25, 2021

@calebcartwright R is just means Redirect, at the company I work, all services are scanned with various tools and scored on different parameters, and a recent service I made simply redirects to other places, so instead of seeing a SH score of e.g. B, I wanted the true score of R to be displayed on the repo.

I will make the changes to ignoreRedirects.

@calebcartwright
Copy link
Member

R is just means Redirect, at the company I work, all services are scanned with various tools and scored on different parameters, and a recent service I made simply redirects to other places, so instead of seeing a SH score of e.g. B, I wanted the true score of R to be displayed on the repo.

If R is just an informational reflection of the fact that it redirects, as opposed to a position in a tiered scale like the others, I wonder if our default badge color for informational message wouldn't be better than the lightgrey (which we use to convey things along the lines of "inactive").

I similarly don't have strong opinions on this one so will go ahead and approve, but will hold off on merging in case others have different views

@shields-cd shields-cd temporarily deployed to shields-staging-pr-6212 February 27, 2021 02:51 Inactive
Co-authored-by: chris48s <chris48s@users.noreply.github.com>
@shields-cd shields-cd temporarily deployed to shields-staging-pr-6212 February 27, 2021 18:02 Inactive
@calebcartwright calebcartwright dismissed their stale review February 27, 2021 19:24

clicked too fast

@shields-cd shields-cd requested a deployment to shields-staging-pr-6212 February 27, 2021 19:26 Abandoned
@repo-ranger repo-ranger bot merged commit 6128aa5 into badges:master Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Security Headers: Expose Follow redirects parameter
6 participants