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

x/pkgsite: add Affected Symbols section to vulnerability pages #54812

Closed
julieqiu opened this issue Sep 1, 2022 · 11 comments
Closed

x/pkgsite: add Affected Symbols section to vulnerability pages #54812

julieqiu opened this issue Sep 1, 2022 · 11 comments
Assignees
Labels
FrozenDueToAge pkgsite UX Issues that involve UXD/UXR input vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Milestone

Comments

@julieqiu
Copy link
Member

julieqiu commented Sep 1, 2022

For example, https://pkg.go.dev/vuln/GO-2021-0113 should have an "Affected Symbols" section explicitly listing these symbols from the ecosystem_specific.imports.symbols section:

"MatchStrings",
"MustParse",
"Parse",
"ParseAcceptLanguage"

@julieqiu julieqiu added pkgsite vulncheck or vulndb Issues for the x/vuln or x/vulndb repo labels Sep 1, 2022
@gopherbot gopherbot added this to the Unreleased milestone Sep 1, 2022
@jamalc jamalc self-assigned this Sep 6, 2022
@neild
Copy link
Contributor

neild commented Sep 6, 2022

@julieqiu julieqiu modified the milestones: pkgsite/unplanned, vuln/2022 Sep 6, 2022
@jamalc jamalc assigned hyangah and unassigned jamalc Sep 8, 2022
@hyangah
Copy link
Contributor

hyangah commented Sep 10, 2022

Blocked by #54992

@hyangah
Copy link
Contributor

hyangah commented Sep 10, 2022

Screen Shot 2022-09-10 at 9 53 21 AM

I am afraid the table view can be a bit too crowed depending on the number of symbols.
Suggestions are welcome.

cc @golang/vulndb

@julieqiu julieqiu added the UX Issues that involve UXD/UXR input label Sep 10, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/429678 mentions this issue: internal/vulns: add affected symbols

@julieqiu
Copy link
Member Author

It might make sense to list the information vertically, as opposed to in a table. It is also likely that in many reports, all exported symbols will be affected.

/cc @jinhongy @fflewddur for UX input

We should also only list exported symbols on this page, and maybe link to the corresponding pkg.go.dev/# page (This is #52660). We compute derived symbols when creating the reports, for this use case on pkgsite.

@hyangah
Copy link
Contributor

hyangah commented Sep 13, 2022

@julieqiu Where is the derived symbols field? I don't see derived symbols in the example data nor the osv package types (https://pkg.go.dev/golang.org/x/vuln/osv#EcosystemSpecificImport).

EDIT: from code I guess the Symbols field contain both derived and originally-reported symbols. But if we exclude all unexported symbols, there will be no exported symbol for net/http for vulnerability GO-2022-0236. Does this case (i.e. symbols is not empty, but there is no exported symbols) mean the entire net/http is vulnerable, or does it mean there is no way to get affected by net/http? Or, is it just a bug in vulndb?

@hyangah
Copy link
Contributor

hyangah commented Sep 14, 2022

This is cl/429678 patchset 3

List affected symbols (exported only) in the next <tr>.
Screen Shot 2022-09-13 at 9 04 41 PM

When multiple packages are affected:

Screen Shot 2022-09-13 at 9 05 07 PM

When there are more than N (e.g. 5) symbols for a package, use <details> or javascript to show more.

Screen Shot 2022-09-13 at 9 04 06 PM

Screen Shot 2022-09-13 at 9 04 24 PM

@jinhongy
Copy link

I would recommend to use the table considered the case that we might have multiple packages with multiple affected symbols. We can make it clear even with huge amount of affected symbols with following adjustment:

  • One symbol for each line
  • Only show max symbols by default, it there are more, add a " # more" link behind
  • Expand the rest of all affected symbols in the table if developers click on the more link

image

@julieqiu
Copy link
Member Author

Thanks @jinhongy! Would you mind sharing what the mobile design would look like? Here's the current mobile view:

Screen Shot 2022-09-16 at 12 31 57 PM

@jinhongy
Copy link

Here is my recommendation for responsive table in mobile view
image

@hyangah
Copy link
Contributor

hyangah commented Sep 17, 2022

Thanks @jinhongy

In the cl/429678 patchset 4, I made it to the three-column (Package, Versions, Symbols) table for wide screens, and made it similar to @jinhongy's mock for narrower screens.
I hope use of grid gives us more flexibility.

To make it look more polished, I need help from @jamalc or @jinhongy who are more capable of js/css.

Wide screen

Mobile view

  • Collapsed

  • Expanded

gopherbot pushed a commit to golang/pkgsite that referenced this issue Sep 21, 2022
Only exported symbols are reported.
In case there are no exported symbols, we present as if all symbols in
the package are completely vulnerable.

Updates golang/go#54812

Change-Id: I4555af8f27ae50fcb1a9e3b9e1c2ec29e750a9ad
Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/429678
Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com>
TryBot-Result: kokoro <noreply+kokoro@google.com>
Reviewed-by: Jamal Carvalho <jamal@golang.org>
Reviewed-by: Jonathan Amsterdam <jba@google.com>
@hyangah hyangah closed this as completed Oct 3, 2022
@hyangah hyangah moved this to Done in Go Security Oct 3, 2022
@golang golang locked and limited conversation to collaborators Oct 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge pkgsite UX Issues that involve UXD/UXR input vulncheck or vulndb Issues for the x/vuln or x/vulndb repo
Projects
Status: Done
Development

No branches or pull requests

6 participants