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

core(csp-xss): hidden severity #12240

Merged
merged 15 commits into from
Mar 24, 2021
Merged

core(csp-xss): hidden severity #12240

merged 15 commits into from
Mar 24, 2021

Conversation

adamraine
Copy link
Member

@adamraine adamraine commented Mar 12, 2021

Can land after #12221

Screen Shot 2021-03-12 at 6 48 25 PM

When there is no CSP, we now use a table item with severity icon instead of the audit display value. If we just use the display value, the audit state for "no csp" will look very similar to the audit state for "csp is all good".

Before:
Screen Shot 2021-03-16 at 12 44 43 PM

After:
Screen Shot 2021-03-16 at 12 43 41 PM

@adamraine adamraine requested a review from a team as a code owner March 12, 2021 23:50
@adamraine adamraine requested review from connorjclark and removed request for a team March 12, 2021 23:50
@google-cla google-cla bot added the cla: yes label Mar 12, 2021
@adamraine
Copy link
Member Author

#12221 (comment) Proposal to move directive column to the left.

@adamraine
Copy link
Member Author

Looks pretty weird for findings with no directive.
Screen Shot 2021-03-15 at 6 04 11 PM

Base automatically changed from csp-eval-npm to master March 16, 2021 20:12
Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed, we'll drop the icons. and yeah lets keep directive as the last column.

you said you wanted to keep some other changes in here, so i'll let you take that.

@connorjclark
Copy link
Collaborator

reminder to change the PR title

@adamraine adamraine changed the title core(csp-xss): severity icons core(csp-xss): hidden severity icons Mar 17, 2021
@adamraine
Copy link
Member Author

Having the icons available in the LHR might still be useful for testing. Additionally, I still think having "No CSP in enforcement mode" as a table item is preferable to having it as the display value.

@brendankenny
Copy link
Member

Having the icons available in the LHR might still be useful for testing.

Since they'll be hidden, can the new icon audit-details type be dropped until we need it?

@adamraine
Copy link
Member Author

Since they'll be hidden, can the new icon audit-details type be dropped until we need it?

I removed the icon code from the report renderer, but I don't think I can remove the type entirely.

@brendankenny
Copy link
Member

couldn't they just be text (e.g. just the current tooltip value) in a "severity" column? e.g. iconName isn't important if it's just for testing for now.

@adamraine adamraine changed the title core(csp-xss): hidden severity icons core(csp-xss): hidden severity Mar 17, 2021
@adamraine
Copy link
Member Author

Could we land this? At this point, it's just a few minor tweaks.

@adamraine adamraine merged commit 20c8080 into master Mar 24, 2021
@adamraine adamraine deleted the csp-severity-icons branch March 24, 2021 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants