Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end SRI tarball integrity verification to the pnpr proxy. A new ChangesTarball Integrity Verification
Sequence Diagram(s)sequenceDiagram
participant Client
participant serve_tarball
participant Storage
participant expected_tarball_integrity
participant verify_file
participant download_verified_to_cache
participant download_verified_to_temp
Client->>serve_tarball: GET /:pkg/-/:tarball.tgz
serve_tarball->>serve_tarball: parse_tarball_name → version
serve_tarball->>Storage: open_hosted_tarball
alt hosted tarball exists
Storage-->>Client: 200 Body (hosted)
end
serve_tarball->>expected_tarball_integrity: packument JSON + version
alt integrity absent or malformed
expected_tarball_integrity-->>serve_tarball: RegistryError::TarballIntegrity
serve_tarball-->>Client: 502 BAD_GATEWAY
end
serve_tarball->>Storage: open_cached_tarball
alt cache hit
serve_tarball->>verify_file: File + Integrity
alt verification OK
serve_tarball-->>Client: 200 Body (cached)
else verification failed
serve_tarball->>Storage: remove_cached_tarball
end
end
serve_tarball->>serve_tarball: select upstream
alt cache:true
serve_tarball->>download_verified_to_cache: Response + TarballWrite + Integrity
download_verified_to_cache-->>serve_tarball: u64 (verified size)
serve_tarball->>Storage: open_cached_tarball + verify_file
serve_tarball-->>Client: 200 Body (newly cached)
else cache:false
serve_tarball->>download_verified_to_temp: Response + TarballWrite + Integrity
download_verified_to_temp-->>serve_tarball: (File, u64, PathBuf)
serve_tarball->>serve_tarball: stream_file_and_remove
serve_tarball-->>Client: 200 Body (temp deleted on Drop)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by Qodofix(pnpr): verify proxied tarball SRI before serving or caching Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1. Hosted errors fail open
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pnpr/crates/pnpr/src/publish.rs (1)
118-123: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueMinor redundancy:
integrity_checkerre-validates afterparse_integrity.Both
parse_integrity(line 118) andintegrity_checker(line 122) callensure_supported_hash. Sinceparse_integrityalready validated the integrity, the second check inintegrity_checkerwill always pass. The duplicate validation is harmless (cheap check, defense-in-depth), but if the streaming API stabilizes, consider whetherintegrity_checkershould accept pre-validatedIntegritywithout re-checking.Not blocking—the shared helper usage is correct per pnpr/AGENTS.md guidance on reusing streaming module functionality.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pnpr/crates/pnpr/src/publish.rs` around lines 118 - 123, Both parse_integrity and integrity_checker are performing redundant validation of the hash algorithm through the shared ensure_supported_hash check. Refactor integrity_checker to accept the already-validated Integrity object directly without re-validating, eliminating the duplicate ensure_supported_hash call. This way, parse_integrity performs the initial validation (lines 118-119), and integrity_checker (line 122) uses the pre-validated Integrity result without repeating the hash type validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pnpr/crates/pnpr/src/publish.rs`:
- Around line 118-123: Both parse_integrity and integrity_checker are performing
redundant validation of the hash algorithm through the shared
ensure_supported_hash check. Refactor integrity_checker to accept the
already-validated Integrity object directly without re-validating, eliminating
the duplicate ensure_supported_hash call. This way, parse_integrity performs the
initial validation (lines 118-119), and integrity_checker (line 122) uses the
pre-validated Integrity result without repeating the hash type validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 24544a2d-8663-482d-8f87-8fb4718424d1
📒 Files selected for processing (13)
pnpr/crates/pnpr/src/error.rspnpr/crates/pnpr/src/package_name.rspnpr/crates/pnpr/src/package_name/tests.rspnpr/crates/pnpr/src/publish.rspnpr/crates/pnpr/src/publish/tests.rspnpr/crates/pnpr/src/server.rspnpr/crates/pnpr/src/storage.rspnpr/crates/pnpr/src/storage/tests.rspnpr/crates/pnpr/src/streaming.rspnpr/crates/pnpr/src/streaming/tests.rspnpr/crates/pnpr/src/upstream.rspnpr/crates/pnpr/src/upstream/tests.rspnpr/crates/pnpr/tests/server.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12570 +/- ##
==========================================
- Coverage 87.99% 87.98% -0.02%
==========================================
Files 324 324
Lines 45693 46008 +315
==========================================
+ Hits 40209 40480 +271
- Misses 5484 5528 +44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Code review by qodo was updated up to the latest commit cd73fcf |
Contributor
Integrated-Benchmark Report (Linux)Each scenario reports direct installs and pnpr installs. Bencher consumes pacquet@HEAD and pnpr@HEAD. Scenario: Isolated linker: fresh restore, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.6795592060199995,
"stddev": 0.20038660622343635,
"median": 4.6458518802199995,
"user": 3.63550204,
"system": 3.4735350999999994,
"min": 4.425094048719999,
"max": 5.06992986772,
"times": [
4.66243183472,
4.425094048719999,
4.944185773719999,
5.06992986772,
4.55825443272,
4.75375199372,
4.6292719257199995,
4.69707970272,
4.478747290719999,
4.576845189719999
]
},
{
"command": "pacquet@main",
"mean": 4.626651276719999,
"stddev": 0.14316109136157926,
"median": 4.60318461422,
"user": 3.63449254,
"system": 3.4876158999999993,
"min": 4.46063669472,
"max": 4.845244065719999,
"times": [
4.55130758172,
4.463902001719999,
4.80607319772,
4.751160500719999,
4.845244065719999,
4.46063669472,
4.70378520372,
4.61078208972,
4.478034292719999,
4.595587138719999
]
},
{
"command": "pnpr@HEAD",
"mean": 3.1461569349199996,
"stddev": 0.20104316405072464,
"median": 3.1739929582200004,
"user": 2.68422094,
"system": 3.0671215,
"min": 2.8921237957200003,
"max": 3.44317086272,
"times": [
3.36264293072,
2.92204585372,
2.8921237957200003,
3.25964375772,
3.30434462172,
3.10815750272,
3.44317086272,
2.96121821472,
2.96839339572,
3.23982841372
]
},
{
"command": "pnpr@main",
"mean": 3.06507530252,
"stddev": 0.12670776995138572,
"median": 3.05324554172,
"user": 2.71942654,
"system": 3.0517996,
"min": 2.88669317972,
"max": 3.2765189607200003,
"times": [
3.02086259572,
2.91864664072,
3.08562848772,
3.2765189607200003,
3.18537888772,
2.88669317972,
2.97925614672,
3.18325460972,
2.99314712372,
3.12136639272
]
}
]
}Scenario: Isolated linker: fresh restore, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.6333493801800001,
"stddev": 0.01712864225501911,
"median": 0.6308284912800001,
"user": 0.37451154000000003,
"system": 1.3199324,
"min": 0.6118490052800001,
"max": 0.6667185222800001,
"times": [
0.6270075582800001,
0.62479264028,
0.62402765128,
0.65394929828,
0.63614091928,
0.63464942428,
0.6667185222800001,
0.6404917842800001,
0.61386699828,
0.6118490052800001
]
},
{
"command": "pacquet@main",
"mean": 0.62411543858,
"stddev": 0.014714645930533362,
"median": 0.6235290152800002,
"user": 0.35974494,
"system": 1.3258869999999998,
"min": 0.60466224428,
"max": 0.6427448652800001,
"times": [
0.6117433102800001,
0.60612321128,
0.6427448652800001,
0.60466224428,
0.63676916128,
0.6284241422800001,
0.63989562328,
0.63793141628,
0.61422652328,
0.6186338882800001
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6930918032800001,
"stddev": 0.049685159417846916,
"median": 0.6795963037800001,
"user": 0.38207114,
"system": 1.3535344,
"min": 0.65865655728,
"max": 0.8319873962800001,
"times": [
0.6903573232800001,
0.6792239212800001,
0.6881666502800001,
0.68058552528,
0.6799686862800001,
0.65865655728,
0.6768022702800001,
0.6662339152800001,
0.8319873962800001,
0.67893578728
]
},
{
"command": "pnpr@main",
"mean": 0.68282543498,
"stddev": 0.016981374733951678,
"median": 0.67485439378,
"user": 0.37912513999999997,
"system": 1.3593102,
"min": 0.6687333332800001,
"max": 0.7197906662800001,
"times": [
0.6719508082800001,
0.66991190828,
0.6873417832800001,
0.67062320328,
0.68651602428,
0.7197906662800001,
0.70367783528,
0.6753347862800001,
0.67437400128,
0.6687333332800001
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + cold store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 4.7022333851199996,
"stddev": 0.04402117614907084,
"median": 4.718608317719999,
"user": 3.7299306999999997,
"system": 3.3928167599999997,
"min": 4.62543423872,
"max": 4.76963329372,
"times": [
4.7218091187199995,
4.69335550872,
4.7275178447199995,
4.73208924372,
4.71565447372,
4.64220900872,
4.62543423872,
4.67306895872,
4.76963329372,
4.72156216172
]
},
{
"command": "pacquet@main",
"mean": 4.748066675719999,
"stddev": 0.03991975848494852,
"median": 4.74719347822,
"user": 3.7549552,
"system": 3.42825166,
"min": 4.68712225972,
"max": 4.83570453572,
"times": [
4.70463606472,
4.68712225972,
4.83570453572,
4.76362525772,
4.76493072272,
4.74581618272,
4.73047275272,
4.74857077372,
4.73956356072,
4.76022464672
]
},
{
"command": "pnpr@HEAD",
"mean": 2.9866253619199994,
"stddev": 0.14096968759975662,
"median": 2.9908365762199995,
"user": 2.5214790999999996,
"system": 2.9733222599999998,
"min": 2.7856358317199996,
"max": 3.17893236872,
"times": [
2.82836920072,
3.00611035272,
3.1299696607199996,
2.7856358317199996,
3.17893236872,
2.9501955727199998,
3.02864660772,
2.9755627997199996,
3.1530322047199997,
2.82979901972
]
},
{
"command": "pnpr@main",
"mean": 3.0386174904199996,
"stddev": 0.17088047876001208,
"median": 3.05617549672,
"user": 2.528645,
"system": 2.9614471599999996,
"min": 2.79566394072,
"max": 3.29901603072,
"times": [
3.0700303657199997,
3.0964564067199998,
3.29901603072,
3.0423206277199997,
3.17201162872,
2.79566394072,
2.83705752772,
3.01515750972,
2.83886949472,
3.21959137172
]
}
]
}Scenario: Isolated linker: fresh install, hot cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 1.32829801322,
"stddev": 0.013412258003630738,
"median": 1.3286762203200002,
"user": 1.32113478,
"system": 1.7189157599999998,
"min": 1.3048131518200001,
"max": 1.3468538378200001,
"times": [
1.32126527882,
1.3248985768200001,
1.3048131518200001,
1.33829975582,
1.3468538378200001,
1.31937737382,
1.3373931618200001,
1.3149924698200002,
1.33245386382,
1.34263266182
]
},
{
"command": "pacquet@main",
"mean": 1.31841244672,
"stddev": 0.025957077302788862,
"median": 1.3164601133200002,
"user": 1.29241808,
"system": 1.7180592599999998,
"min": 1.29297896682,
"max": 1.38562057982,
"times": [
1.32247035782,
1.30047171582,
1.38562057982,
1.29297896682,
1.32066810882,
1.3137695898200001,
1.31243572682,
1.2961647438200001,
1.3203940408200001,
1.31915063682
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6556501811200002,
"stddev": 0.03646384359794694,
"median": 0.6426393008200001,
"user": 0.33694368,
"system": 1.2673569599999999,
"min": 0.63362321182,
"max": 0.7545972498200001,
"times": [
0.63522718582,
0.63527517582,
0.6443781928200001,
0.6472002088200001,
0.63362321182,
0.7545972498200001,
0.6380582988200001,
0.6653532528200001,
0.6409004088200001,
0.6618886258200001
]
},
{
"command": "pnpr@main",
"mean": 0.65454320412,
"stddev": 0.009322232553246,
"median": 0.65372205782,
"user": 0.3490266799999999,
"system": 1.27489616,
"min": 0.6391529918200001,
"max": 0.6747791288200001,
"times": [
0.6581635998200001,
0.6391529918200001,
0.6571308108200001,
0.6480278928200001,
0.6524505958200001,
0.6549935198200001,
0.6485423908200001,
0.6598453218200001,
0.6747791288200001,
0.65234578882
]
}
]
}Scenario: Isolated linker: fresh install, cold cache + hot store
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.9948598004,
"stddev": 0.06224688629895745,
"median": 2.9982920924,
"user": 1.7605566799999999,
"system": 1.9524906400000002,
"min": 2.9171887579,
"max": 3.1025843619,
"times": [
2.9870747249,
2.9250697149,
2.9395866428999997,
2.9494404379,
2.9171887579,
3.0095094599,
3.0142966509,
3.1025843619,
3.0582653008999996,
3.0455819518999996
]
},
{
"command": "pacquet@main",
"mean": 2.9644247318,
"stddev": 0.010868956089893144,
"median": 2.9669181793999995,
"user": 1.72465108,
"system": 1.9520985400000002,
"min": 2.9449760489,
"max": 2.9846274549,
"times": [
2.9449760489,
2.9678517048999997,
2.9846274549,
2.9573335549,
2.9659846538999997,
2.9543938629,
2.9682012318999997,
2.9598028439,
2.9713621529,
2.9697138089
]
},
{
"command": "pnpr@HEAD",
"mean": 0.6534543752,
"stddev": 0.008504248932055198,
"median": 0.6544355524000001,
"user": 0.33510158,
"system": 1.2873143400000002,
"min": 0.6352037009,
"max": 0.6663926819,
"times": [
0.6663926819,
0.6569638379,
0.6563039339000001,
0.6493861109000001,
0.6352037009,
0.6540182659,
0.6623830239,
0.6500034959000001,
0.6548528389,
0.6490358619000001
]
},
{
"command": "pnpr@main",
"mean": 0.6623714879,
"stddev": 0.04948526330751643,
"median": 0.6473817779000001,
"user": 0.33870718,
"system": 1.2787692400000001,
"min": 0.6389349879,
"max": 0.8018539199000001,
"times": [
0.6412989779,
0.6389349879,
0.6394614409,
0.6490664459000001,
0.6413096039,
0.6522470989000001,
0.6585096709,
0.6553356229,
0.6456971099000001,
0.8018539199000001
]
}
]
} |
Contributor
|
Bencher Report| Branch | pr/12570 |
| Testbed | pacquet |
Click to view all benchmark results
| Benchmark | Latency | Benchmark Result milliseconds (ms) (Result Δ%) | Upper Boundary milliseconds (ms) (Limit %) |
|---|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 4,702.23 ms(+11.57%)Baseline: 4,214.56 ms | 5,057.48 ms (92.98%) |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot 🚷 view threshold | 2,994.86 ms(-0.31%)Baseline: 3,004.19 ms | 3,605.03 ms (83.07%) |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 1,328.30 ms(+0.24%)Baseline: 1,325.07 ms | 1,590.08 ms (83.54%) |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot 🚷 view threshold | 4,679.56 ms(+18.50%)Baseline: 3,948.99 ms | 4,738.79 ms (98.75%) |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot 🚷 view threshold | 633.35 ms(+1.77%)Baseline: 622.31 ms | 746.77 ms (84.81%) |
Contributor
|
| Branch | pr/12570 |
| Testbed | pnpr |
⚠️ WARNING: No Threshold found!Without a Threshold, no Alerts will ever be generated.
Click here to create a new Threshold
For more information, see the Threshold documentation.
To only post results if a Threshold exists, set the--ci-only-thresholdsflag.
Click to view all benchmark results
| Benchmark | Latency | milliseconds (ms) |
|---|---|---|
| isolated-linker.fresh-install.cold-cache.cold-store | 📈 view plot | 2,986.63 ms |
| isolated-linker.fresh-install.cold-cache.hot-store | 📈 view plot | 653.45 ms |
| isolated-linker.fresh-install.hot-cache.hot-store | 📈 view plot | 655.65 ms |
| isolated-linker.fresh-restore.cold-cache.cold-store | 📈 view plot | 3,146.16 ms |
| isolated-linker.fresh-restore.hot-cache.hot-store | 📈 view plot | 693.09 ms |
|
Code review by qodo was updated up to the latest commit ae4de9b |
1 similar comment
|
Code review by qodo was updated up to the latest commit ae4de9b |
|
Code review by qodo was updated up to the latest commit c2b106f |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pnpr/crates/pnpr/src/package_name/tests.rs (1)
7-9: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAssert parsed tuple values, not only parse success.
These calls only verify that parsing doesn’t error; they don’t prove the extracted version/canonical filename contract.
Suggested test tightening
fn accepts_unscoped() { let name = PackageName::parse("lodash").unwrap(); assert_eq!(name.as_str(), "lodash"); assert_eq!(name.tarball_name_for_version("4.17.21"), "lodash-4.17.21.tgz"); - name.parse_tarball_name("lodash-4.17.21.tgz").unwrap(); + assert_eq!( + name.parse_tarball_name("lodash-4.17.21.tgz").unwrap(), + ("lodash-4.17.21.tgz".to_string(), "4.17.21".to_string()), + ); } #[test] fn accepts_scoped() { let name = PackageName::parse("`@types/node`").unwrap(); assert_eq!(name.as_str(), "`@types/node`"); assert_eq!(name.tarball_name_for_version("20.0.0"), "node-20.0.0.tgz"); - name.parse_tarball_name("node-20.0.0.tgz").unwrap(); + assert_eq!( + name.parse_tarball_name("node-20.0.0.tgz").unwrap(), + ("node-20.0.0.tgz".to_string(), "20.0.0".to_string()), + ); }As per coding guidelines, “ensure tests prove the regression/changed behavior (not just execution).”
Also applies to: 13-16
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pnpr/crates/pnpr/src/package_name/tests.rs` around lines 7 - 9, The test currently only verifies that parse_tarball_name succeeds by calling unwrap without asserting the actual parsed values. Capture the result of parse_tarball_name("lodash-4.17.21.tgz").unwrap() into a variable and add assertions to verify the returned tuple contains the expected extracted values (version and canonical name). Apply the same fix to all similar parse_tarball_name calls in the test to ensure the parsed contract is properly validated, not just that parsing completes without error.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pnpr/crates/pnpr/src/package_name/tests.rs`:
- Around line 7-9: The test currently only verifies that parse_tarball_name
succeeds by calling unwrap without asserting the actual parsed values. Capture
the result of parse_tarball_name("lodash-4.17.21.tgz").unwrap() into a variable
and add assertions to verify the returned tuple contains the expected extracted
values (version and canonical name). Apply the same fix to all similar
parse_tarball_name calls in the test to ensure the parsed contract is properly
validated, not just that parsing completes without error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 15523dde-f37c-4b86-aba5-29d80ef4c3dd
📒 Files selected for processing (13)
pnpr/crates/pnpr/src/error.rspnpr/crates/pnpr/src/package_name.rspnpr/crates/pnpr/src/package_name/tests.rspnpr/crates/pnpr/src/publish.rspnpr/crates/pnpr/src/publish/tests.rspnpr/crates/pnpr/src/server.rspnpr/crates/pnpr/src/storage.rspnpr/crates/pnpr/src/storage/tests.rspnpr/crates/pnpr/src/streaming.rspnpr/crates/pnpr/src/streaming/tests.rspnpr/crates/pnpr/src/upstream.rspnpr/crates/pnpr/src/upstream/tests.rspnpr/crates/pnpr/tests/server.rs
✅ Files skipped from review due to trivial changes (1)
- pnpr/crates/pnpr/src/upstream/tests.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- pnpr/crates/pnpr/src/publish/tests.rs
- pnpr/crates/pnpr/src/error.rs
- pnpr/crates/pnpr/src/streaming/tests.rs
- pnpr/crates/pnpr/src/publish.rs
- pnpr/crates/pnpr/src/upstream.rs
- pnpr/crates/pnpr/src/streaming.rs
- pnpr/crates/pnpr/src/server.rs
- pnpr/crates/pnpr/tests/server.rs
|
Code review by qodo was updated up to the latest commit 3299fb3 |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
|
Code review by qodo was updated up to the latest commit 5d98c4f |
|
Code review by qodo was updated up to the latest commit 4b04173 |
Code Review by QodoNew Review StartedThis review has been superseded by a new analysis |
Bind proxied tarball requests to the selected packument version. Require supported dist.integrity before serving upstream or cached tarballs. Verify cache hits before response construction. Fail closed on a hosted-store fault instead of falling through to the upstream proxy, so an I/O error in the authoritative store can never serve bytes of a different provenance for the same package name. Delete invalid cache entries and promote upstream bytes only after SRI verification. For cache:false uplinks, verify into a temp file and stream the same open handle (rewound to the start) instead of dropping and reopening it by path, closing the TOCTOU window where an attacker-writable cache directory could swap the verified bytes before they are served; remove the temp file after streaming. Harden publish attachment SRI parsing for missing or unsupported integrity. Addresses GHSA-5f9g-98vq-2jxw.
|
Code review by qodo was updated up to the latest commit 16d9850 |
1 similar comment
|
Code review by qodo was updated up to the latest commit 16d9850 |
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Closed
1 task
zkochan
added a commit
that referenced
this pull request
Jun 22, 2026
#12570 made pnpr download a proxied tarball in full, hash it, fsync and rename it into the cache, then reopen and reread it before responding. That serialized the upstream and client transfers (doubling the latency-bound time per tarball) and added a full extra disk round-trip, regressing the cold-cache integrated benchmarks by ~0.5s. Restore concurrent streaming: tee each upstream chunk to the client as it arrives while hashing it into a temp file, and promote the temp file into the proxy cache only when the complete body matches the declared SRI. The cache therefore still never stores unverified bytes, but the proxy no longer waits for the whole body before the first byte reaches the client. Authoritative verification of installed bytes remains the client's own SRI check against the packument; the proxy guards only what it persists. The structural guarantees from #12570 are kept: hosted-store fail-closed, request-to-version binding, and the cache-hit integrity sidecar fast path. Replace download_verified_to_cache with tee_verified_to_cache and drop the reopen/reread on the cache-miss path.
4 tasks
zkochan
added a commit
that referenced
this pull request
Jun 22, 2026
… the packument #12570 added, to the tarball *serve* path, a packument load + full JSON parse (to derive the expected integrity) plus a sidecar read and a conditional re-hash — on every request, before the cache lookup. Pre-#12570 a cached/proxied tarball was streamed straight from disk with none of that. On a warm proxy cache that per-serve work is the cold-store regression, not the cache-miss buffering: the integrated benchmark serves from a warm pnpr cache, so the streaming-tee change alone moved nothing. Take the cache lookup first and trust the integrity sidecar: when a cached tarball has a sidecar whose recorded length matches the file, the bytes were already verified against the packument integrity when they were stored (the cache-miss tee only promotes on an SRI match), so serve them directly without re-loading or re-parsing the packument and without re-hashing. The packument is now resolved lazily — only for a cache entry lacking a trustworthy sidecar (verified once, then the sidecar is recorded) and for cache misses (the verified fetch needs it). The client still verifies the bytes it installs against its own resolved integrity, so the proxy guards what it writes, not every read.
zkochan
added a commit
that referenced
this pull request
Jun 22, 2026
The proxied-tarball integrity fix (#12570, GHSA-5f9g-98vq-2jxw) started reconstructing each rewritten dist.tarball filename from its version key (`<name>-<version>.tgz`). That assumes upstream tarball names are always canonical, which breaks packages like esprima-fb whose real npm tarball is `esprima-fb-3001.0001.0000-dev-harmony-fb.tgz` for version `3001.1.0-dev-harmony-fb`: the rewritten URL no longer matched what npm hosts, so the client recorded the wrong lockfile URL and the proxied fetch 404'd. Preserve the upstream basename verbatim in the rewrite again, and resolve a tarball request's version and dist.integrity by matching the requested filename against each version's dist.tarball basename instead of parsing the version out of the filename. OSV screening re-runs against the resolved version when it differs from the filename-derived one. The GHSA protection is unchanged: served bytes are still verified against the SRI declared by the version that owns that tarball name, so a preserved name cannot smuggle in bytes of another provenance. Tests that encoded the canonical-reconstruction behavior are updated to assert basename preservation, with new coverage for non-canonical names. Also fix a pre-existing compile break in the pnpr server test target: UserStore::in_memory gained a MaxUsers argument (#12581) but one test call site was not updated.
2 tasks
zkochan
added a commit
that referenced
this pull request
Jun 29, 2026
The proxy-cache tarball serve path re-hashed each cached `.tgz` against the version's `dist.integrity` on every request (#12570). On a cold-store install the client pulls ~1300 tarballs from pnpr, so the accelerator paid a full SRI pass per tarball — the cold-store regression visible in Bencher from the auth-era change cluster onward (~2.1s -> ~2.9s, persistent). The re-hash is redundant. A tarball only enters the cache through `download_verified_to_cache`, which verifies the bytes against `dist.integrity` as they are written, so nothing unverified is ever stored. Every install client also re-verifies the tarball it receives against that same integrity. Serve the cached bytes directly instead. Write-time verification (and rejection of a poisoned upstream tarball, never caching it) is preserved, so GHSA-5f9g-98vq-2jxw stays mitigated. The namespaced `/~<uplink>/` route keeps its sidecar-keyed integrity cache and is unaffected. Drops the now-dead `record_cached_tarball_integrity`/`discard_cached_tarball` helpers and the per-serve cached-tarball integrity sidecar.
2 tasks
zkochan
added a commit
that referenced
this pull request
Jun 29, 2026
Removing only the on-read SRI re-hash recovered part of the cold-store regression but not all of it: #12570 also made every tarball serve load and parse the package's packument to bind the request to a version and its `dist.integrity`. On a warm-cache cold-store install (~1300 tarballs pulled from pnpr) that is a packument parse per tarball — the larger half of the regression. The cache hit needs neither binding. A tarball only enters the proxy cache through `download_verified_to_cache`, which resolves the version and verifies the bytes against `dist.integrity` as they are written, so the cache only ever holds correct bytes for a given (name, filename); the install client re-verifies whatever it receives against its own expected integrity regardless. The OSV screen on the filename's version still runs before the cache read. So serve cached bytes directly and defer the packument load to the cache-miss download path, which still needs the integrity to verify the freshly-fetched bytes before caching them. This matches the pre-regression cache-hit serve cost while keeping write-time verification (which the pre-regression path lacked), so the bytes that enter the cache are still verified and GHSA-5f9g-98vq-2jxw stays mitigated.
zkochan
added a commit
that referenced
this pull request
Jun 29, 2026
…from its own pnpr mock The integrated benchmark fronts all arms with a single registry-mock built from the PR's HEAD (`just integrated-benchmark` runs `cargo build --release --bin=pnpr`, and the mock resolves that one `target/release/pnpr`). Tarball serving — where the pnpr proxy spends its per-request time — happens in that shared mock, not in the per-revision `pnpr@<rev>` accelerators (which only offload resolution; clients fetch tarballs from the client registry). So a tarball-serve change slows or speeds every arm equally and cancels out of the `pnpr@HEAD` vs `pnpr@main` comparison — the blind spot that let the serve-path regression in #12570 merge looking flat and only step up on the `main` trend afterward. Give each compared revision its own tarball-serving mock, built from that revision's `pnpr`. `plan_revision_mocks` assigns a client-facing latency-proxy port to every revision that has a `pnpr@<rev>` target (so its binary is built) before `init()` bakes the port into each target's `.npmrc`; `benchmark()` then spawns the mock (after `build()` produced the binary, after `init()` warmed the shared storage) and fronts it with the same latency + bandwidth link the shared mock uses. `pacquet@<rev>` and `pnpr@<rev>` of the same revision share that revision's mock, so the pnpr-vs-direct ratio stays fair within a revision while the serve-path delta becomes visible across revisions. All revisions share the one warm runtime storage: serving a warm cache is read-only (a hit neither re-downloads nor, after #12709, writes a sidecar), so concurrent mocks don't contend. Non-Verdaccio modes plan no mock and keep the existing single-registry behavior. Also retry `rm -rf` on the transient "Directory not empty" macOS/APFS raises between a just-finished install's store writes settling and the next iteration's cleanup, so the benchmark can run locally on macOS at all.
zkochan
added a commit
that referenced
this pull request
Jun 29, 2026
- Drop the refactor-history reference (what #12570 added and why the old serve path was slow) from the cache-hit comment in serve_tarball; keep the current invariant (write-time + client verification) that explains why a hit can serve directly. That history belongs in the commit log. - Put the "per-revision mock makes a serve-path delta visible (a shared mock cancels it out)" rationale in one place — `plan_revision_mocks` — and have `registry_for`, `benchmark()`, and `pnpr_command_with_binary` reference or state it briefly instead of re-deriving it. - Drop the "served unscreened before this fix" framing from the OSV cache-hit test comment; describe the current behavior instead.
zkochan
added a commit
that referenced
this pull request
Jun 29, 2026
…ve path (#12709) #12570 made every tarball serve — warm cache hits included — both re-hash the cached bytes against dist.integrity and load+parse the packument to bind the request to a version. The benchmark mock is pnpr and a cold-store install pulls ~1300 tarballs through it, so both costs are paid per tarball: together the cold-store regression from ~2.15s to ~3.15s on the Bencher pnpr testbed (#12700 recovered only the resolution-cache part). Both are redundant on a cache hit. A tarball only enters the proxy cache via download_verified_to_cache, which resolves the version and verifies the bytes against dist.integrity as they are written, so the cache only ever holds correct bytes for a (name, filename); the install client re-verifies whatever it receives anyway. Serve cached bytes directly and defer the packument load to the cache-miss download path, which still verifies freshly-fetched bytes before caching them. The OSV screen on the filename version still runs first. Write-time verification is preserved, so GHSA-5f9g-98vq-2jxw stays mitigated — the bytes that enter the cache are still verified, which the pre-regression serve path did not even do. Drops the now-dead per-serve integrity sidecar and helpers.
This was referenced Jun 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
dist.integritybefore serving or caching them.cache: falseuplinks.pacquet/port is needed; this change is scoped to the pnpr registry server.Addresses https://github.com/pnpm/pnpm/security/advisories/GHSA-5f9g-98vq-2jxw.
Squash Commit Body
Checklist
Written by an agent (Codex, GPT-5).
Summary by CodeRabbit
Release Notes
dist.integrity. Missing, malformed, or unsupported values now fail with gateway-style errors and prevent serving/caching corrupted content. Absent versions return404.