From ff5b00587cb4a966ceb0233cbee469b3f1387f76 Mon Sep 17 00:00:00 2001 From: Helge Sverre Date: Tue, 4 Aug 2026 10:27:17 +0200 Subject: [PATCH] fix(github): read sema_version_req from sema.toml at each tag sync_tag took the requirement as a parameter, and only the link handler passed one. The sync endpoint and the webhook both passed None, so every release after the initial link stored NULL. The client reads NULL as "no requirement", which means the gate never fired for the normal release flow of a GitHub-linked package. The link path was wrong in the other direction: it applied the default branch sema.toml to every historic tag, so editing the field rewrote the requirement of past releases. sync_tag now fetches sema.toml at the tag itself and drops the parameter, so all three callers record what each release actually declared. The webhook has no authenticated user, so it reads the repo with an owner stored GitHub token via the new dal::owners::first_user_id. A tag with no readable sema.toml records no requirement, because tags older than the manifest must still import. A manifest that parses but holds an invalid requirement is an error recorded in the sync log, so the maintainer sees it instead of the release silently losing its constraint. sync_tag now takes RepoAccess rather than four more positional &str arguments, which also satisfies clippy::too_many_arguments. Tests: tag_sema_version_req is split out as a pure function and covered for the valid, absent, unparsable, invalid, and non-string cases. Adds an integration test that pins the sema_version_req JSON key end to end through publish and GET, which is the break that would silently disable the client check, and one that a bad requirement is refused without creating the package. --- README.md | 6 ++ src/api/github.rs | 72 ++++++++++++----------- src/dal/owners.rs | 20 +++++++ src/github_sync.rs | 119 ++++++++++++++++++++++++++++++++++++-- tests/integration_test.rs | 75 ++++++++++++++++++++++++ 5 files changed, 255 insertions(+), 37 deletions(-) diff --git a/README.md b/README.md index bb59e64..a7d798d 100644 --- a/README.md +++ b/README.md @@ -218,6 +218,12 @@ GitHub-linked releases. Clients use it to skip incompatible versions during unversioned installs and to reject incompatible explicit or locked installs before replacing local package files. +For a GitHub-linked package the requirement is read from `sema.toml` **at each +tag**, so every release records the requirement it was published with and +editing the field does not rewrite past releases. A tag whose `sema.toml` is +missing or unreadable records no requirement; a tag whose `sema.toml` parses but +holds an invalid requirement is reported in the package's sync log. + ### Source Locking A package is either **CLI-uploaded** or **GitHub-linked**, never both. Once a package is linked to a repo, it cannot be published via `sema publish`, and vice versa. diff --git a/src/api/github.rs b/src/api/github.rs index 0dba41e..0c7d2af 100644 --- a/src/api/github.rs +++ b/src/api/github.rs @@ -123,18 +123,14 @@ pub async fn link( let mut imported = 0u32; let mut errors = Vec::new(); + let access = github_sync::RepoAccess { + client: &client, + token: &token, + owner: &owner_name, + repo: &repo, + }; for (tag_name, version) in &tags { - match github_sync::sync_tag( - &state.db, - &owner_name, - &repo, - tag_name, - version, - package_id, - manifest.sema_version_req.as_deref(), - ) - .await - { + match github_sync::sync_tag(&state.db, &access, tag_name, version, package_id).await { Ok(true) => imported += 1, Ok(false) => {} Err(e) => { @@ -210,18 +206,14 @@ pub async fn sync( }; let mut imported = 0u32; + let access = github_sync::RepoAccess { + client: &client, + token: &token, + owner: &owner_name, + repo: &repo, + }; for (tag_name, version) in &tags { - match github_sync::sync_tag( - &state.db, - &owner_name, - &repo, - tag_name, - version, - package_id, - None, - ) - .await - { + match github_sync::sync_tag(&state.db, &access, tag_name, version, package_id).await { Ok(true) => imported += 1, Ok(false) => {} Err(e) => { @@ -339,17 +331,31 @@ pub async fn webhook( let (owner_name, repo) = github_sync::parse_github_url(repo_full_name) .ok_or_else(|| ApiError::bad_request("Invalid repo name"))?; - match github_sync::sync_tag( - &state.db, - &owner_name, - &repo, - tag_name, - &version, - package_id, - None, - ) - .await - { + // The webhook has no authenticated user, so read the repo with an owner's stored + // GitHub token. Without it the tag's sema_version_req cannot be read at all. + let owner_user_id = dal::owners::first_user_id(&state.db, package_id) + .await + .ok() + .flatten() + .ok_or_else(|| ApiError::forbidden("Package has no owner"))?; + let token = + github_sync::get_github_token(&state.db, owner_user_id, &state.config.oauth_token_key) + .await + .ok_or_else(|| { + ApiError::new( + StatusCode::BAD_GATEWAY, + "Package owner has no active GitHub connection", + ) + })?; + let client = reqwest::Client::new(); + let access = github_sync::RepoAccess { + client: &client, + token: &token, + owner: &owner_name, + repo: &repo, + }; + + match github_sync::sync_tag(&state.db, &access, tag_name, &version, package_id).await { Ok(true) => { tracing::info!("Webhook: synced {repo_full_name} tag {tag_name} as {version}"); crate::audit::log( diff --git a/src/dal/owners.rs b/src/dal/owners.rs index f6e4981..8e26278 100644 --- a/src/dal/owners.rs +++ b/src/dal/owners.rs @@ -40,6 +40,26 @@ pub async fn package_id_if_owner( Ok(row.and_then(|r| r.try_get("", "id").ok())) } +/// The lowest owner user id for `package_id`, if the package has an owner. +/// +/// The webhook runs with no authenticated user, so it needs an owner's stored GitHub +/// token to read the repo. Ordering by id keeps the choice stable as owners change. +pub async fn first_user_id( + db: &C, + package_id: i64, +) -> Result, DbErr> { + let row = db + .query_one(crate::db::stmt( + db.get_database_backend(), + r#"SELECT o.user_id FROM owners o + WHERE o.package_id = ? + ORDER BY o.user_id ASC"#, + [package_id.into()], + )) + .await?; + Ok(row.and_then(|r| r.try_get("", "user_id").ok())) +} + /// Usernames of every owner of `package_id`. pub async fn list_usernames( db: &C, diff --git a/src/github_sync.rs b/src/github_sync.rs index 07884ae..916e1bc 100644 --- a/src/github_sync.rs +++ b/src/github_sync.rs @@ -169,19 +169,87 @@ pub async fn list_semver_tags( Ok(tags) } +/// Everything needed to read one GitHub repository. +/// +/// Grouping these keeps the owner, repo, and tag from being transposed at a call site +/// where they are all `&str`. +pub struct RepoAccess<'a> { + pub client: &'a reqwest::Client, + pub token: &'a str, + pub owner: &'a str, + pub repo: &'a str, +} + +/// Read `[package].sema_version_req` from `sema.toml` at one tag. +/// +/// A tag with no readable `sema.toml` declares no requirement — tags older than the +/// manifest are normal, and a fetch failure must not block the import. A manifest that +/// IS readable but holds an invalid requirement is an error, so the maintainer sees it +/// in the sync log instead of the release silently losing its constraint. +async fn fetch_tag_sema_version_req( + access: &RepoAccess<'_>, + tag_name: &str, +) -> Result, String> { + let RepoAccess { + client, + token, + owner, + repo, + } = access; + let url = format!("https://api.github.com/repos/{owner}/{repo}/contents/sema.toml"); + let resp = client + .get(url) + // `.query` percent-encodes, so a tag like `release/1.0` stays one parameter. + .query(&[("ref", tag_name)]) + .header("Authorization", format!("Bearer {token}")) + .header("User-Agent", "sema-pkg") + .header("Accept", "application/vnd.github.raw+json") + .send() + .await; + let Ok(resp) = resp else { + return Ok(None); + }; + if !resp.status().is_success() { + return Ok(None); + } + let Ok(content) = resp.text().await else { + return Ok(None); + }; + tag_sema_version_req(&content, tag_name) +} + +/// Extract and validate `[package].sema_version_req` from one tag's `sema.toml`. +/// +/// An unparsable manifest declares no requirement: `sema.toml` is not required to be +/// valid TOML at every historic tag, and refusing the tag would block the import. A +/// manifest that parses but holds a bad requirement is an error, so the maintainer sees +/// it instead of the release silently losing its constraint. +fn tag_sema_version_req(content: &str, tag_name: &str) -> Result, String> { + let Ok(document) = toml::from_str::(content) else { + return Ok(None); + }; + let Some(value) = document + .get("package") + .and_then(|package| package.get("sema_version_req")) + else { + return Ok(None); + }; + let requirement = value + .as_str() + .ok_or_else(|| format!("{tag_name}: sema_version_req must be a string"))?; + validate_sema_version_req(Some(requirement)).map_err(|error| format!("{tag_name}: {error}")) +} + /// Sync a single tag: store metadata and GitHub tarball URL (no blob download). /// Returns Ok(true) if version was created, Ok(false) if it already existed. pub async fn sync_tag( db: &Db, - owner: &str, - repo: &str, + access: &RepoAccess<'_>, tag_name: &str, version: &semver::Version, package_id: i64, - sema_version_req: Option<&str>, ) -> Result { let version_str = version.to_string(); - let sema_version_req = validate_sema_version_req(sema_version_req)?; // Check if version already exists let exists = crate::dal::versions::exists(db, package_id, &version_str) @@ -192,6 +260,11 @@ pub async fn sync_tag( return Ok(false); } + // Read the requirement from the manifest at this tag, not from the default branch: + // each release states its own, and editing the field must not rewrite past releases. + let sema_version_req = fetch_tag_sema_version_req(access, tag_name).await?; + + let (owner, repo) = (access.owner, access.repo); let tarball_url = format!("https://api.github.com/repos/{owner}/{repo}/tarball/{tag_name}"); crate::dal::versions::create_github_version( @@ -507,6 +580,44 @@ mod manifest_tests { assert_eq!(error, "sema_version_req must be a string"); } + #[test] + fn tag_manifest_reads_the_requirement_at_that_tag() { + let content = "[package]\nname = \"policies\"\nsema_version_req = \" >=1.34.0 \"\n"; + assert_eq!( + tag_sema_version_req(content, "v1.0.0").unwrap(), + Some(">=1.34.0".to_string()) + ); + } + + #[test] + fn tag_manifest_without_a_requirement_is_absent_not_an_error() { + // A tag older than the field, or older than sema.toml itself, must still import. + assert_eq!( + tag_sema_version_req("[package]\nname = \"policies\"\n", "v0.1.0").unwrap(), + None + ); + assert_eq!(tag_sema_version_req("", "v0.1.0").unwrap(), None); + assert_eq!( + tag_sema_version_req("not : valid : toml", "v0.1.0").unwrap(), + None + ); + } + + #[test] + fn tag_manifest_with_a_bad_requirement_names_the_tag() { + let error = tag_sema_version_req( + "[package]\nsema_version_req = \"not a requirement\"\n", + "v1.2.3", + ) + .unwrap_err(); + assert!(error.starts_with("v1.2.3: "), "{error}"); + assert!(error.contains("Invalid sema_version_req"), "{error}"); + + let error = + tag_sema_version_req("[package]\nsema_version_req = 34\n", "v1.2.3").unwrap_err(); + assert_eq!(error, "v1.2.3: sema_version_req must be a string"); + } + #[test] fn validator_treats_blank_as_absent_and_limits_length() { assert_eq!(validate_sema_version_req(Some(" ")).unwrap(), None); diff --git a/tests/integration_test.rs b/tests/integration_test.rs index 977d3ff..55eac2d 100644 --- a/tests/integration_test.rs +++ b/tests/integration_test.rs @@ -248,6 +248,81 @@ async fn test_publish_and_get_package() { assert_eq!(body["owners"][0], "publisher"); } +#[tokio::test] +async fn test_sema_version_req_survives_publish_and_is_served_under_that_key() { + // The `sema pkg` client reads versions[].sema_version_req from this response and + // refuses an incompatible install. If the key is renamed, dropped, or left out of + // the query, the client silently stops enforcing, so pin the name and the value. + let (app, _dir) = test_app().await; + let session = register_user(app.clone(), "reqpub", "req@example.com").await; + let token = create_api_token(app.clone(), &session, "req-token").await; + + let meta = serde_json::json!({ + "description": "requirement carrier", + "sema_version_req": " >=1.34.0 ", + }); + let res = publish_package_full( + app.clone(), + &token, + "req-pkg", + "1.0.0", + &gzip(b"tarball"), + &serde_json::to_string(&meta).unwrap(), + ) + .await; + assert_eq!(res.status(), StatusCode::CREATED); + + let res = app + .clone() + .oneshot( + Request::builder() + .uri("/api/v1/packages/req-pkg") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(res.status(), StatusCode::OK); + let body = body_json(res).await; + // Stored normalized (trimmed), served under exactly this key. + assert_eq!(body["versions"][0]["sema_version_req"], ">=1.34.0"); +} + +#[tokio::test] +async fn test_publish_rejects_an_invalid_sema_version_req() { + let (app, _dir) = test_app().await; + let session = register_user(app.clone(), "badreq", "badreq@example.com").await; + let token = create_api_token(app.clone(), &session, "badreq-token").await; + + let meta = serde_json::json!({ + "description": "bad requirement", + "sema_version_req": "not a requirement", + }); + let res = publish_package_full( + app.clone(), + &token, + "badreq-pkg", + "1.0.0", + &gzip(b"tarball"), + &serde_json::to_string(&meta).unwrap(), + ) + .await; + assert_eq!(res.status(), StatusCode::BAD_REQUEST); + + // The rejected publish must not have created the package. + let res = app + .clone() + .oneshot( + Request::builder() + .uri("/api/v1/packages/badreq-pkg") + .body(Body::empty()) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(res.status(), StatusCode::NOT_FOUND); +} + #[tokio::test] async fn test_publish_duplicate_version() { let (app, _dir) = test_app().await;