fix(profiles): improve validation and address review feedback

polaz · polaz · commit cf7fa578c89b · 2026-01-20T15:12:02.000+02:00
- Improve denied_actions validation to check both tool and action parts
  are non-empty (fixes ':action', 'tool:', ':' edge cases)
- Fix getProfileNameFromEnv JSDoc comment accuracy
- Move multiple --profile warning outside loop with count
- Add tests for denied_actions edge cases
diff --git a/src/main.ts b/src/main.ts
@@ -23,11 +23,14 @@ function getProfileFromArgs(): string | undefined {
       profileCount++;
       if (profileCount === 1) {
         profileName = value;
-      } else if (profileCount === 2) {
-        logger.warn("Multiple --profile flags detected, using first value");
       }
     }
   }
+
+  if (profileCount > 1) {
+    logger.warn({ count: profileCount }, "Multiple --profile flags detected, using first value");
+  }
+
   return profileName;
 }
 
diff --git a/src/profiles/loader.ts b/src/profiles/loader.ts
@@ -335,11 +335,18 @@ export class ProfileLoader {
       }
     }
 
-    // Validate denied_actions format
+    // Validate denied_actions format (must be 'tool:action' with non-empty parts)
     if (profile.denied_actions) {
       for (const action of profile.denied_actions) {
-        if (!action.includes(":")) {
+        const colonIndex = action.indexOf(":");
+        if (colonIndex === -1) {
           errors.push(`Invalid denied_action format '${action}', expected 'tool:action'`);
+        } else {
+          const tool = action.slice(0, colonIndex).trim();
+          const act = action.slice(colonIndex + 1).trim();
+          if (!tool || !act) {
+            errors.push(`Invalid denied_action format '${action}', expected 'tool:action'`);
+          }
         }
       }
     }
@@ -367,11 +374,18 @@ export class ProfileLoader {
       }
     }
 
-    // Validate denied_actions format
+    // Validate denied_actions format (must be 'tool:action' with non-empty parts)
     if (preset.denied_actions) {
       for (const action of preset.denied_actions) {
-        if (!action.includes(":")) {
+        const colonIndex = action.indexOf(":");
+        if (colonIndex === -1) {
           errors.push(`Invalid denied_action format '${action}', expected 'tool:action'`);
+        } else {
+          const tool = action.slice(0, colonIndex).trim();
+          const act = action.slice(colonIndex + 1).trim();
+          if (!tool || !act) {
+            errors.push(`Invalid denied_action format '${action}', expected 'tool:action'`);
+          }
         }
       }
     }
@@ -431,7 +445,7 @@ export async function loadPreset(name: string): Promise<Preset> {
 }
 
 /**
- * Get profile name from CLI args or environment
+ * Get profile name from GITLAB_PROFILE environment variable
  */
 export function getProfileNameFromEnv(): string | undefined {
   return process.env.GITLAB_PROFILE;
diff --git a/tests/unit/profiles/loader.test.ts b/tests/unit/profiles/loader.test.ts
@@ -493,6 +493,45 @@ describe("ProfileLoader", () => {
       expect(result.errors.some(e => e.includes("Invalid denied_action format"))).toBe(true);
     });
 
+    it("should error on denied_actions with empty tool part", async () => {
+      const profile: Profile = {
+        host: "gitlab.example.com",
+        auth: { type: "pat", token_env: "TOKEN" },
+        denied_actions: [":action"], // Empty tool
+      };
+
+      const result = await loader.validateProfile(profile);
+
+      expect(result.valid).toBe(false);
+      expect(result.errors.some(e => e.includes("Invalid denied_action format"))).toBe(true);
+    });
+
+    it("should error on denied_actions with empty action part", async () => {
+      const profile: Profile = {
+        host: "gitlab.example.com",
+        auth: { type: "pat", token_env: "TOKEN" },
+        denied_actions: ["tool:"], // Empty action
+      };
+
+      const result = await loader.validateProfile(profile);
+
+      expect(result.valid).toBe(false);
+      expect(result.errors.some(e => e.includes("Invalid denied_action format"))).toBe(true);
+    });
+
+    it("should error on denied_actions with only colon", async () => {
+      const profile: Profile = {
+        host: "gitlab.example.com",
+        auth: { type: "pat", token_env: "TOKEN" },
+        denied_actions: [":"], // Both empty
+      };
+
+      const result = await loader.validateProfile(profile);
+
+      expect(result.valid).toBe(false);
+      expect(result.errors.some(e => e.includes("Invalid denied_action format"))).toBe(true);
+    });
+
     it("should error when SSL cert path does not exist", async () => {
       const profile: Profile = {
         host: "gitlab.example.com",
@@ -659,6 +698,23 @@ describe("ProfileLoader", () => {
       expect(result.valid).toBe(false);
       expect(result.errors.some(e => e.includes("Invalid denied_action format"))).toBe(true);
     });
+
+    it("should error on preset denied_actions with empty parts", async () => {
+      // Test all edge cases: empty tool, empty action, both empty
+      const testCases = [":action", "tool:", ":"];
+
+      for (const action of testCases) {
+        const preset: Preset = {
+          denied_actions: [action],
+          features: {},
+        };
+
+        const result = await loader.validatePreset(preset);
+
+        expect(result.valid).toBe(false);
+        expect(result.errors.some(e => e.includes("Invalid denied_action format"))).toBe(true);
+      }
+    });
   });
 
   describe("clearCache", () => {

Original file line number	Diff line number	Diff line change
`@@ -23,11 +23,14 @@ function getProfileFromArgs(): string \| undefined {`
`23`	`23`	`profileCount++;`
`24`	`24`	`if (profileCount === 1) {`
`25`	`25`	`profileName = value;`
`26`		`- } else if (profileCount === 2) {`
`27`		`- logger.warn("Multiple --profile flags detected, using first value");`
`28`	`26`	`}`
`29`	`27`	`}`
`30`	`28`	`}`
	`29`	`+`
	`30`	`+ if (profileCount > 1) {`
	`31`	`+ logger.warn({ count: profileCount }, "Multiple --profile flags detected, using first value");`
	`32`	`+ }`
	`33`	`+`
`31`	`34`	`return profileName;`
`32`	`35`	`}`
`33`	`36`
Original file line number	Diff line number	Diff line change
`@@ -335,11 +335,18 @@ export class ProfileLoader {`
`335`	`335`	`}`
`336`	`336`	`}`
`337`	`337`
`338`		`- // Validate denied_actions format`
	`338`	`+ // Validate denied_actions format (must be 'tool:action' with non-empty parts)`
`339`	`339`	`if (profile.denied_actions) {`
`340`	`340`	`for (const action of profile.denied_actions) {`
`341`		`- if (!action.includes(":")) {`
	`341`	`+ const colonIndex = action.indexOf(":");`
	`342`	`+ if (colonIndex === -1) {`
`342`	`343`	errors.push(`Invalid denied_action format '${action}', expected 'tool:action'`);
	`344`	`+ } else {`
	`345`	`+ const tool = action.slice(0, colonIndex).trim();`
	`346`	`+ const act = action.slice(colonIndex + 1).trim();`
	`347`	`+ if (!tool \|\| !act) {`
	`348`	+ errors.push(`Invalid denied_action format '${action}', expected 'tool:action'`);
	`349`	`+ }`
`343`	`350`	`}`
`344`	`351`	`}`
`345`	`352`	`}`
`@@ -367,11 +374,18 @@ export class ProfileLoader {`
`367`	`374`	`}`
`368`	`375`	`}`
`369`	`376`
`370`		`- // Validate denied_actions format`
	`377`	`+ // Validate denied_actions format (must be 'tool:action' with non-empty parts)`
`371`	`378`	`if (preset.denied_actions) {`
`372`	`379`	`for (const action of preset.denied_actions) {`
`373`		`- if (!action.includes(":")) {`
	`380`	`+ const colonIndex = action.indexOf(":");`
	`381`	`+ if (colonIndex === -1) {`
`374`	`382`	errors.push(`Invalid denied_action format '${action}', expected 'tool:action'`);
	`383`	`+ } else {`
	`384`	`+ const tool = action.slice(0, colonIndex).trim();`
	`385`	`+ const act = action.slice(colonIndex + 1).trim();`
	`386`	`+ if (!tool \|\| !act) {`
	`387`	+ errors.push(`Invalid denied_action format '${action}', expected 'tool:action'`);
	`388`	`+ }`
`375`	`389`	`}`
`376`	`390`	`}`
`377`	`391`	`}`
`@@ -431,7 +445,7 @@ export async function loadPreset(name: string): Promise<Preset> {`
`431`	`445`	`}`
`432`	`446`
`433`	`447`	`/**`
`434`		`- * Get profile name from CLI args or environment`
	`448`	`+ * Get profile name from GITLAB_PROFILE environment variable`
`435`	`449`	`*/`
`436`	`450`	`export function getProfileNameFromEnv(): string \| undefined {`
`437`	`451`	`return process.env.GITLAB_PROFILE;`