Skip to content

Conversation

@djc
Copy link
Contributor

@djc djc commented Nov 12, 2025

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.

Screenshot 2025-11-12 at 11 53 29

@djc djc marked this pull request as draft November 12, 2025 10:56
@djc djc changed the title Initial tab at a security tab showing advisories Initial stab at a security tab showing advisories Dec 1, 2025
@djc djc force-pushed the security-tab branch 2 times, most recently from 118a13c to 0ad81ca Compare December 12, 2025 16:02
@djc djc marked this pull request as ready for review December 12, 2025 16:02
@djc
Copy link
Contributor Author

djc commented Dec 12, 2025

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?

Copy link
Member

@Turbo87 Turbo87 left a 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.

@Turbo87 Turbo87 added the C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works label Dec 15, 2025
@djc djc force-pushed the security-tab branch 4 times, most recently from 4aa6fdf to a6f7be2 Compare December 17, 2025 15:58
Comment on lines +20 to +23
{{else if @controller.error}}
<div class='no-results' data-error>
An error occurred while fetching advisories.
</div>
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason for not using the replaceWith('catch-all', ...) pattern suggested in #12311 (comment)? that way we get consistent error page styling across the application and a "Try Again" button to retry the failed request.

<a href='https://rustsec.org/advisories/{{advisory.id}}.html'>{{advisory.id}}</a>:
{{advisory.summary}}
</h3>
{{htmlSafe (@controller.convertMarkdown advisory.details)}}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should add a couple of smoke tests for the micromark library, to ensure that is sanitizes correctly. if the library changes their defaults we should probably have some kind of notification mechanism to let us know about it (e.g. a failing test case).

Comment on lines +28 to +43
// Check first advisory
await expect(page.locator('[data-test-list] li').nth(0).locator('h3 a')).toHaveAttribute(
'href',
'https://rustsec.org/advisories/TEST-001.html',
);
await expect(page.locator('[data-test-list] li').nth(0).locator('h3 a')).toContainText('TEST-001');
await expect(page.locator('[data-test-list] li').nth(0).locator('h3')).toContainText('First test advisory');
await expect(page.locator('[data-test-list] li').nth(0).locator('p')).toContainText('markdown');

// Check second advisory
await expect(page.locator('[data-test-list] li').nth(1).locator('h3 a')).toHaveAttribute(
'href',
'https://rustsec.org/advisories/TEST-002.html',
);
await expect(page.locator('[data-test-list] li').nth(1).locator('h3 a')).toContainText('TEST-002');
await expect(page.locator('[data-test-list] li').nth(1).locator('h3')).toContainText('Second test advisory');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Check first advisory
await expect(page.locator('[data-test-list] li').nth(0).locator('h3 a')).toHaveAttribute(
'href',
'https://rustsec.org/advisories/TEST-001.html',
);
await expect(page.locator('[data-test-list] li').nth(0).locator('h3 a')).toContainText('TEST-001');
await expect(page.locator('[data-test-list] li').nth(0).locator('h3')).toContainText('First test advisory');
await expect(page.locator('[data-test-list] li').nth(0).locator('p')).toContainText('markdown');
// Check second advisory
await expect(page.locator('[data-test-list] li').nth(1).locator('h3 a')).toHaveAttribute(
'href',
'https://rustsec.org/advisories/TEST-002.html',
);
await expect(page.locator('[data-test-list] li').nth(1).locator('h3 a')).toContainText('TEST-002');
await expect(page.locator('[data-test-list] li').nth(1).locator('h3')).toContainText('Second test advisory');
// Check first advisory
let advisory1 = page.locator('[data-test-list] li').nth(0);
await expect(advisory1.locator('h3 a')).toHaveAttribute('href', 'https://rustsec.org/advisories/TEST-001.html');
await expect(advisory1.locator('h3 a')).toHaveText('TEST-001');
await expect(advisory1.locator('h3')).toContainText('First test advisory');
expect(await advisory1.locator('p').innerHTML()).toBe(
'This is the first test advisory with <strong>markdown</strong> support.',
);
// Check second advisory
let advisory2 = page.locator('[data-test-list] li').nth(1);
await expect(advisory2.locator('h3 a')).toHaveAttribute('href', 'https://rustsec.org/advisories/TEST-002.html');
await expect(advisory2.locator('h3 a')).toHaveText('TEST-002');
await expect(advisory2.locator('h3')).toContainText('Second test advisory');
expect(await advisory2.locator('p').innerHTML()).toBe('This is the second test advisory with more details.');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-frontend 🐹 C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Tracking issue for "Adding a crates.io Security tab"

4 participants