Skip to content
Sign up

flagon-io/g1t

public
00

Where people and agents ship software together. The open-source git platform for the whole job: issues, agents, checks and deploys to the edge.

gitagentscloudflarerustforgemcp

Commit

Security keeps the id and workspace of each moved work item

work.migrated rewrote what security kept by old issue numbers to the new numbers only, and every lookup then found the number in the repository's workspace now: after a transfer, another workspace's item of the same number. Migration 0009 keeps each item's id and workspace beside its number, a lookup uses that workspace and accepts only the item of that id, and the catch-up replays the map so rows rewritten before get their ids.

G1T-Session:
ses_01m56w5m6cegebbtcjh7ax0a5g
syntaqxcommitted Parent2aa6bc5Browse files
5 files+167−220/5 viewed
+18−0
1+-- What security keeps by a work item's number also keeps the item's id and
2+-- workspace, from `work.migrated` (g1t #57): a repository transferred to
3+-- another workspace still finds the item it had, never the new
4+-- workspace's item of the same number.
5+ALTER TABLE updates ADD COLUMN item_id TEXT;
6+ALTER TABLE updates ADD COLUMN item_workspace TEXT;
7+ALTER TABLE update_pulls ADD COLUMN item_id TEXT;
8+ALTER TABLE update_pulls ADD COLUMN item_workspace TEXT;
9+ALTER TABLE upgrades ADD COLUMN item_id TEXT;
10+ALTER TABLE upgrades ADD COLUMN item_workspace TEXT;
11+ALTER TABLE code_alerts ADD COLUMN item_id TEXT;
12+ALTER TABLE code_alerts ADD COLUMN item_workspace TEXT;
13+ALTER TABLE alert_fixes ADD COLUMN item_id TEXT;
14+ALTER TABLE alert_fixes ADD COLUMN item_workspace TEXT;
15+-- Pages already applied rewrote the numbers without the ids: the cron's
16+-- catch-up reads the whole map again, and each page fills in the ids of
17+-- rows already on their new numbers (store.rs `WORK_MIGRATED`).
18+UPDATE work_catch_up SET cursor = NULL, page = 0, done_at = NULL WHERE name = 'work.migrated';
+22−8
4141 use crate::deps::advisory_table;
4242 use crate::pull_text;
4343 use crate::ranges;
44−use crate::store::{Activity, RepoRow, UpdateRow, VulnRow};
44+use crate::store::{Activity, LinkedItem, RepoRow, UpdateRow, VulnRow};
4545 use crate::update_store::NewPull;
4646 use crate::version_updates::{Loaded, package_ecosystem};
4747
270270 repo.namespace.to_lowercase()
271271 }
272272
273+ /// The work item a row names, as g1t sees it: found in the workspace
274+ /// it was kept with (the repository may have moved since), and only if
275+ /// it is still the item of the id kept for it.
276+ pub(crate) async fn linked(&self, repo: &RepoRow, linked: &LinkedItem) -> Result<Option<g1t_contracts::work::WorkItem>> {
277+ let workspace = Self::workspace_of(repo);
278+ let found = self.item(&User::system(&repo.namespace), linked.workspace_or(&workspace), linked.number).await?;
279+ Ok(found.filter(|item| linked.is(&item.id)))
280+ }
281+
273282 /// Closes a work item g1t opened for an update before it stopped
274− /// opening them, saying why. One already closed is left as it is.
275− pub(crate) async fn close_item_with(&self, repo: &RepoRow, number: u32, why: String) -> Result<()> {
283+ /// opening them, saying why. One already closed is left as it is, and
284+ /// one that is no longer the item kept for it is never touched.
285+ pub(crate) async fn close_item_with(&self, repo: &RepoRow, linked: &LinkedItem, why: String) -> Result<()> {
286+ if self.linked(repo, linked).await?.is_none() {
287+ return Ok(());
288+ }
276289 let system = User::system(&repo.namespace);
277− let workspace = Self::workspace_of(repo);
290+ let workspace = linked.workspace_or(&Self::workspace_of(repo)).to_owned();
291+ let number = linked.number;
278292 let item = WorkItemActionArgs { actor: system.clone(), workspace: workspace.clone(), number, reason: Some(CloseReason::NotPlanned) };
279293 let closed: Outcome<Value> =
280294 g1t_kit::call(&self.work, "close_work_item", &CloseWorkItemArgs { item, repo: None, commit: None }).await?;
356370 None => {
357371 // An upgrade work item from before security updates,
358372 // still open, is left to the agent on it.
359− if let Some(existing) = self.store.upgrade(&repo.repo_id, &ecosystem, &package).await?
373+ if let Some(existing) = self.store.upgrade(&repo.repo_id, &ecosystem, &package).await?.and_then(|row| row.linked_item())
360374 && self
361− .item(&User::system(&repo.namespace), &Self::workspace_of(repo), existing.item as u32)
375+ .linked(repo, &existing)
362376 .await?
363377 .is_some_and(|item| item.state == g1t_contracts::work::State::Open)
364378 {
664678 // make: the work item closes, and the update fails as one would now.
665679 for row in self.store.needs_code_updates(10).await? {
666680 let why = cannot_raise(&row.package, &row.target);
667− if let (Some(repo), Some(number)) = (self.store.repo(&row.repo_id).await?, row.item()) {
681+ if let (Some(repo), Some(linked)) = (self.store.repo(&row.repo_id).await?, row.linked_item()) {
668682 let path = Self::path_of(&repo);
669683 let alerts = format!("/{}/{}/security/vulnerabilities", path.namespace, path.name);
670− self.close_item_with(&repo, number, format!(
684+ self.close_item_with(&repo, &linked, format!(
671685 "g1t no longer opens work items for upgrades it cannot make on its own. This one is on [its alert]({alerts}), \
672686 which says why and is tried again later."
673687 ))
+117−10
114114
115115 /// Secrets with how many places each was found.
116116 /// Rewrites the work items kept by the old issue numbers of their
117−/// repositories to their numbers in the workspace, `?1` a page of
118−/// `work.migrated`'s items as JSON. Each new number is above every old one
119−/// of its workspace, so a page heard twice changes nothing more.
117+/// repositories to their numbers in the workspace, keeping each item's id
118+/// and workspace with it, `?1` a page of `work.migrated`'s items as JSON.
119+/// Each new number is above every old one of its workspace, so a page heard
120+/// twice changes nothing more; a row already on its new number from before
121+/// the ids were kept (migration 0009) gets them.
120122 pub const WORK_MIGRATED: [&str; 6] = [
121− "UPDATE updates SET item = m.number FROM moved_items m WHERE updates.repo_id = m.repo_id AND updates.item = m.legacy",
122− "UPDATE update_pulls SET item = m.number FROM moved_items m WHERE update_pulls.repo_id = m.repo_id AND update_pulls.item = m.legacy",
123− "UPDATE upgrades SET item = m.number FROM moved_items m WHERE upgrades.repo_id = m.repo_id AND upgrades.item = m.legacy",
124− "UPDATE code_alerts SET item = m.number FROM moved_items m WHERE code_alerts.repo_id = m.repo_id AND code_alerts.item = m.legacy",
125− "UPDATE alert_fixes SET item = m.number FROM moved_items m WHERE alert_fixes.repo_id = m.repo_id AND alert_fixes.item = m.legacy",
123+ "UPDATE updates SET item = m.number, item_id = m.item_id, item_workspace = m.workspace FROM moved_items m
124+ WHERE updates.repo_id = m.repo_id AND (updates.item = m.legacy OR (updates.item = m.number AND updates.item_id IS NULL))",
125+ "UPDATE update_pulls SET item = m.number, item_id = m.item_id, item_workspace = m.workspace FROM moved_items m
126+ WHERE update_pulls.repo_id = m.repo_id AND (update_pulls.item = m.legacy OR (update_pulls.item = m.number AND update_pulls.item_id IS NULL))",
127+ "UPDATE upgrades SET item = m.number, item_id = m.item_id, item_workspace = m.workspace FROM moved_items m
128+ WHERE upgrades.repo_id = m.repo_id AND (upgrades.item = m.legacy OR (upgrades.item = m.number AND upgrades.item_id IS NULL))",
129+ "UPDATE code_alerts SET item = m.number, item_id = m.item_id, item_workspace = m.workspace FROM moved_items m
130+ WHERE code_alerts.repo_id = m.repo_id AND (code_alerts.item = m.legacy OR (code_alerts.item = m.number AND code_alerts.item_id IS NULL))",
131+ "UPDATE alert_fixes SET item = m.number, item_id = m.item_id, item_workspace = m.workspace FROM moved_items m
132+ WHERE alert_fixes.repo_id = m.repo_id AND (alert_fixes.item = m.legacy OR (alert_fixes.item = m.number AND alert_fixes.item_id IS NULL))",
126133 // Only an update that needed code changes named a work item; the
127134 // other steps name pull requests, which kept their numbers.
128135 "UPDATE alert_activity SET number = m.number FROM moved_items m
132139 /// `work.migrated`'s items (`?1`), as the rows [`WORK_MIGRATED`] reads.
133140 const MOVED_ITEMS: &str = "WITH moved_items AS (
134141 SELECT json_extract(value, '$.repoId') AS repo_id, json_extract(value, '$.legacyNumber') AS legacy,
135− json_extract(value, '$.number') AS number
142+ json_extract(value, '$.number') AS number, json_extract(value, '$.itemId') AS item_id,
143+ json_extract(value, '$.workspace') AS workspace
136144 FROM json_each(?1))";
137145
146+/// The work item a row names: its number, and, from `work.migrated`, its
147+/// id and the workspace it is in.
148+#[derive(Clone, Debug, PartialEq, Eq)]
149+pub struct LinkedItem {
150+ pub number: u32,
151+ pub item_id: Option<String>,
152+ pub workspace: Option<String>,
153+}
154+
155+impl LinkedItem {
156+ pub fn new(number: Option<i64>, item_id: Option<&str>, workspace: Option<&str>) -> Option<Self> {
157+ number.map(|n| LinkedItem { number: n.max(0) as u32, item_id: item_id.map(str::to_owned), workspace: workspace.map(str::to_owned) })
158+ }
159+
160+ /// The workspace to find it in: the one it was kept with, else the
161+ /// repository's now (`repo_workspace`), for a row from before.
162+ pub fn workspace_or<'a>(&'a self, repo_workspace: &'a str) -> &'a str {
163+ self.workspace.as_deref().unwrap_or(repo_workspace)
164+ }
165+
166+ /// Whether the item found by its number is this one: with its id kept,
167+ /// only the item of that id is.
168+ pub fn is(&self, found_id: &str) -> bool {
169+ self.item_id.as_deref().is_none_or(|id| id == found_id)
170+ }
171+}
172+
138173 /// One of [`WORK_MIGRATED`], ready to run.
139174 pub fn work_migrated_sql(statement: &str) -> String {
140175 format!("{MOVED_ITEMS}
254289 pub branch: Option<String>,
255290 pub pull: Option<i64>,
256291 pub item: Option<i64>,
292+ #[serde(default)]
293+ pub item_id: Option<String>,
294+ #[serde(default)]
295+ pub item_workspace: Option<String>,
257296 pub error: Option<String>,
258297 pub updated_at: String,
259298 }
260299
261300 impl UpdateRow {
301+ pub fn linked_item(&self) -> Option<LinkedItem> {
302+ LinkedItem::new(self.item, self.item_id.as_deref(), self.item_workspace.as_deref())
303+ }
304+
262305 pub fn state(&self) -> UpdateState {
263306 UpdateState::parse(&self.state).unwrap_or(UpdateState::Failed)
264307 }
337380 #[derive(Deserialize)]
338381 pub struct UpgradeRow {
339382 pub item: i64,
383+ #[serde(default)]
384+ pub item_id: Option<String>,
385+ #[serde(default)]
386+ pub item_workspace: Option<String>,
387+}
388+
389+impl UpgradeRow {
390+ pub fn linked_item(&self) -> Option<LinkedItem> {
391+ LinkedItem::new(Some(self.item), self.item_id.as_deref(), self.item_workspace.as_deref())
392+ }
340393 }
341394
342395 #[derive(Deserialize)]
11541207 /// The work item opened for a package before security updates, if any.
11551208 pub async fn upgrade(&self, repo_id: &str, ecosystem: &str, package: &str) -> Result<Option<UpgradeRow>> {
11561209 self.db
1157− .prepare("SELECT item FROM upgrades WHERE repo_id = ? AND ecosystem = ? AND package = ?")
1210+ .prepare("SELECT item, item_id, item_workspace FROM upgrades WHERE repo_id = ? AND ecosystem = ? AND package = ?")
11581211 .bind(&[repo_id.into(), ecosystem.into(), package.into()])?
11591212 .first::<UpgradeRow>(None)
11601213 .await
12901343 }
12911344
12921345 #[test]
1346+ fn work_migrated_keeps_each_items_id_and_workspace() {
1347+ let db = rusqlite::Connection::open_in_memory().unwrap();
1348+ let (before, after): (Vec<_>, Vec<_>) = migrations().into_iter().partition(|(name, _)| name.as_str() < "0009");
1349+ for (name, sql) in &before {
1350+ db.execute_batch(sql).unwrap_or_else(|error| panic!("{name}: {error}"));
1351+ }
1352+ // rep_1's update is still on its old number; rep_2's upgrade was
1353+ // rewritten by a page heard before ids were kept, and the catch-up
1354+ // was done.
1355+ db.execute_batch(
1356+ "INSERT INTO updates (repo_id, ecosystem, package, target, state, pull, item, requested_at, updated_at)
1357+ VALUES ('rep_1', 'npm', 'a', '2', 'needs_code', NULL, 5, '', '');
1358+ INSERT INTO upgrades (repo_id, ecosystem, package, item, target, opened_at) VALUES ('rep_2', 'npm', 'b', 40, '2', '');
1359+ UPDATE work_catch_up SET page = 3, done_at = 'x';",
1360+ )
1361+ .unwrap();
1362+ for (name, sql) in &after {
1363+ db.execute_batch(sql).unwrap_or_else(|error| panic!("{name}: {error}"));
1364+ }
1365+ // The catch-up reads the map again, for the ids of rows already moved.
1366+ let done: Option<String> = db.query_row("SELECT done_at FROM work_catch_up", [], |row| row.get(0)).unwrap();
1367+ assert_eq!(done, None);
1368+ let page = serde_json::json!([
1369+ { "repoId": "rep_1", "legacyNumber": 5, "workspace": "acme", "number": 41, "itemId": "iss_5" },
1370+ { "repoId": "rep_2", "legacyNumber": 3, "workspace": "acme", "number": 40, "itemId": "iss_3" },
1371+ ])
1372+ .to_string();
1373+ for _ in 0..2 {
1374+ for statement in WORK_MIGRATED {
1375+ db.execute(&work_migrated_sql(statement), [&page]).unwrap();
1376+ }
1377+ }
1378+ let linked = |sql: &str| {
1379+ db.query_row(sql, [], |row| Ok((row.get::<_, i64>(0)?, row.get::<_, Option<String>>(1)?, row.get::<_, Option<String>>(2)?)))
1380+ .unwrap()
1381+ };
1382+ assert_eq!(linked("SELECT item, item_id, item_workspace FROM updates"), (41, Some("iss_5".into()), Some("acme".into())));
1383+ assert_eq!(linked("SELECT item, item_id, item_workspace FROM upgrades"), (40, Some("iss_3".into()), Some("acme".into())));
1384+ }
1385+
1386+ #[test]
1387+ fn a_linked_item_is_found_where_it_was_kept() {
1388+ let kept = LinkedItem::new(Some(41), Some("iss_5"), Some("acme")).unwrap();
1389+ // The repository moved to another workspace: its item did not.
1390+ assert_eq!(kept.workspace_or("globex"), "acme");
1391+ assert!(kept.is("iss_5"));
1392+ // Another item that has the number now is not it.
1393+ assert!(!kept.is("wki_9"));
1394+ let before = LinkedItem::new(Some(41), None, None).unwrap();
1395+ assert_eq!(before.workspace_or("globex"), "globex");
1396+ assert!(before.is("wki_9"));
1397+ }
1398+
1399+ #[test]
12931400 fn only_real_secrets_in_history_are_open() {
12941401 let counts = secret_counts(
12951402 [("open", false, 2), ("open", true, 3), ("blocked", false, 1), ("blocked", true, 4), ("allowed", true, 5), ("resolved", false, 6)]
+6−2
4545 /// The work item g1t opened for it, from before it stopped doing that.
4646 #[serde(default)]
4747 pub item: Option<i64>,
48+ #[serde(default)]
49+ pub item_id: Option<String>,
50+ #[serde(default)]
51+ pub item_workspace: Option<String>,
4852 pub head: Option<String>,
4953 pub merge_by: Option<String>,
5054 pub error: Option<String>,
6064 self.pull.map(|n| n.max(0) as u32)
6165 }
6266
63− pub fn item(&self) -> Option<u32> {
64− self.item.map(|n| n.max(0) as u32)
67+ pub fn linked_item(&self) -> Option<crate::store::LinkedItem> {
68+ crate::store::LinkedItem::new(self.item, self.item_id.as_deref(), self.item_workspace.as_deref())
6569 }
6670
6771 pub fn dependencies(&self) -> Vec<UpdatedDependency> {
+4−2
403403 // failed: the work item closes, and the next run opens the update again
404404 // as a pull request that stays open with its failing checks.
405405 for row in self.store.needs_code_update_pulls(10).await? {
406− if let (Some(repo), Some(number)) = (self.store.repo(&row.repo_id).await?, row.item()) {
406+ if let (Some(repo), Some(linked)) = (self.store.repo(&row.repo_id).await?, row.linked_item()) {
407407 let why = "g1t no longer opens work items for updates whose checks fail. The entry's next run opens this update again \
408408 as a pull request, which stays open with its checks for you to decide on.";
409− self.close_item_with(&repo, number, why.to_owned()).await?;
409+ self.close_item_with(&repo, &linked, why.to_owned()).await?;
410410 }
411411 let error = "Raising the versions failed this branch's required checks.";
412412 self.store.set_update_pull(&row.id, UpdateState::Failed, row.pull(), None, Some(error)).await?;
15871587 state: state.into(),
15881588 pull: Some(3),
15891589 item: None,
1590+ item_id: None,
1591+ item_workspace: None,
15901592 head: None,
15911593 merge_by: None,
15921594 error: None,

Comments

Sign in to comment.