-
Notifications
You must be signed in to change notification settings - Fork 683
Initial stab at a security tab showing advisories #12311
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
base: main
Are you sure you want to change the base?
Conversation
|
I've implemented Markdown formatting with GitHub-flavored Markdown support for the advisory details. Seems like a decent MVP from which we can iterate further? |
Turbo87
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a good start! 👍
one more suggestion that did not fit into the diff: we should add tests for this new feature to https://github.com/rust-lang/crates.io/tree/main/e2e/acceptance. at least one test for displaying 2+ advisories, one for the empty state, and one for the rustsec server returning e.g. a 500 error. we use msw for network mocking, which should make it relatively straight-forward to implement. it's probably sufficient to implement the msw request handlers in the tests instead of creating a generic handler in the crates-io-msw package.
| @service releaseTracks; | ||
| @service sentry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| @service releaseTracks; | |
| @service sentry; |
these services are not used by anything AFAICT, so I think we can safely remove them
| import Controller from '@ember/controller'; | ||
| import { service } from '@ember/service'; | ||
| import { tracked } from '@glimmer/tracking'; | ||
|
|
||
| export default class SecurityController extends Controller { | ||
| @service releaseTracks; | ||
| @service sentry; | ||
|
|
||
| @tracked crate; | ||
| @tracked data; | ||
|
|
||
| constructor() { | ||
| super(...arguments); | ||
| this.reset(); | ||
| } | ||
|
|
||
| reset() { | ||
| this.crate = undefined; | ||
| this.data = undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| import Controller from '@ember/controller'; | |
| import { service } from '@ember/service'; | |
| import { tracked } from '@glimmer/tracking'; | |
| export default class SecurityController extends Controller { | |
| @service releaseTracks; | |
| @service sentry; | |
| @tracked crate; | |
| @tracked data; | |
| constructor() { | |
| super(...arguments); | |
| this.reset(); | |
| } | |
| reset() { | |
| this.crate = undefined; | |
| this.data = undefined; | |
| } | |
| } |
Since we explicitly assign the controller fields in the setupController() fn in the route, I don't think we need an explicit controller implementation at all and can just rely on the automatically generated empty one.
| // reset when crate changes | ||
| if (crate && crate !== controller.crate) { | ||
| controller.reset(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // reset when crate changes | |
| if (crate && crate !== controller.crate) { | |
| controller.reset(); | |
| } |
The reset() fn resets the crate and advisories fields, but, since we immediately reassign them below, I don't think we need the reset() call, right?
| {{/each}} | ||
| </ul> | ||
| {{else}} | ||
| <div class='no-results'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no corresponding styling block in the CSS file, or did I miss it? 😅
| queryParams = { | ||
| sort: { refreshModel: true }, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| queryParams = { | |
| sort: { refreshModel: true }, | |
| }; |
The sort query param does not appear to be used anywhere, so I guess we can remove it? I assume the JSON file that we request is not dynamically generated, so that probably won't support different sort orders anyway.
| return { crate, advisories, convertMarkdown }; | ||
| } catch (error) { | ||
| // report unexpected errors to Sentry and ignore `ajax()` errors | ||
| if (!didCancel(error) && !(error instanceof AjaxError)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the didCancel() is only relevant for ember-concurrency tasks, but since we don't use such tasks here we don't need to check for it.
the AjaxError is an error thrown by the ajax() utility function in this project, but fetchAdvisories() is using raw fetch() instead. we have two options here: 1. use ajax() instead of fetch() and catch the 404 error in fetchAdvisories(), or 2. we check for the "HTTP error" error message prefix from the custom error thrown in fetchAdvisories().
if an error is thrown in the try block above we currently only handle it by reporting it to Sentry, but we then implicitly return undefined; since there is no other handling of the error. since setupController() is using destructuring, and destructuring of undefined won't work, this will probably lead to further errors. instead we should show a proper error page like we do for most of the other routes. an example of how this can be accomplished is https://github.com/rust-lang/crates.io/blob/main/app/routes/crate/version-dependencies.js#L34-L35.
| .row { | ||
| margin-top: var(--space-2xs); | ||
| background-color: light-dark(white, #141413); | ||
| padding: var(--space-m) var(--space-l); | ||
| list-style: none; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .row { | |
| margin-top: var(--space-2xs); | |
| background-color: light-dark(white, #141413); | |
| padding: var(--space-m) var(--space-l); | |
| list-style: none; | |
| } | |
| .row { | |
| margin-top: var(--space-2xs); | |
| background-color: light-dark(white, #141413); | |
| border-radius: var(--space-3xs); | |
| padding: var(--space-m) var(--space-l); | |
| list-style: none; | |
| } |
We often add a little rounding to it.
Here's my first take at implementing the security tab as proposed in RFC 3872. Resolves #12507.
This is intentionally light on details to get an initial review on my first baby Ember.js steps. Not sure how much we need to add before this gets merged.
Keeping as draft while the RFC has not been merged.