Skip to content

feat: find credentials in args and headers#98

Merged
lirantal merged 2 commits intomainfrom
feat/find-creds-in-args-and-headers
Sep 27, 2025
Merged

feat: find credentials in args and headers#98
lirantal merged 2 commits intomainfrom
feat/find-creds-in-args-and-headers

Conversation

@lirantal
Copy link
Copy Markdown
Owner

@lirantal lirantal commented Sep 27, 2025

User description

Description

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

  • I have updated the documentation (if required).
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • I added a picture of a cute animal cause it's fun

PR Type

Enhancement


Description

  • Expand credential detection to command arguments and HTTP headers

  • Add variable substitution safety detection for patterns like ${input:token}

  • Implement source-specific visual indicators with icons

  • Enhance credential warning display with grouped source information


Diagram Walkthrough

flowchart LR
  A["MCP Server Config"] --> B["Environment Variables"]
  A --> C["Command Arguments"] 
  A --> D["HTTP Headers"]
  B --> E["Credential Detection Service"]
  C --> E
  D --> E
  E --> F["Variable Substitution Check"]
  F --> G["Risk Assessment"]
  G --> H["Source-Grouped Display"]
Loading

File Walkthrough

Relevant files
Tests
credential-detection-integration.test.ts
Add comprehensive integration tests for multi-source credential
detection

tests/credential-detection-integration.test.ts

  • Add integration tests for argument credential detection (--api-key
    TOKEN)
  • Add tests for HTTP header credential detection with variable
    substitution handling
  • Add comprehensive server config tests combining all credential sources
  • Add edge case tests for empty/missing data scenarios
+103/-0 
credential-detection-service.test.ts
Add extensive unit tests for new credential detection methods

tests/credential-detection-service.test.ts

  • Add unit tests for variable substitution pattern detection
  • Add comprehensive tests for analyzeArguments method with various flag
    patterns
  • Add tests for analyzeHeaders method with authorization and API key
    headers
  • Add tests for analyzeServerConfig method combining all sources
+234/-0 
Enhancement
credential-warning.ts
Enhance credential warning with source-grouped display and icons

src/components/credential-warning.ts

  • Group credentials by source (env/args/headers) for organized display
  • Add source-specific icons (🌱 env, ⚡ args, 🔗 headers)
  • Update credential summary format to show source grouping
  • Add getSourceIcon helper function for visual indicators
+35/-5   
credential-detection-service.ts
Implement multi-source credential detection with variable substitution
safety

src/services/credential-detection-service.ts

  • Add ARGUMENT_PATTERNS and HEADER_PATTERNS for detecting credentials in
    args/headers
  • Implement isVariableSubstitution method to detect safe patterns like
    ${input:token}
  • Add analyzeArguments method for command-line credential detection
  • Add analyzeHeaders method for HTTP header credential detection
  • Add analyzeServerConfig method for comprehensive multi-source analysis
  • Update analyzeEnvironmentVariables to skip variable substitution
    patterns
  • Add source field to CredentialVariable interface
+219/-1 
mcp-config-service.ts
Integrate multi-source credential analysis in config service

src/services/mcp-config-service.ts

  • Add headers field to server info extraction
  • Update credential analysis to use comprehensive analyzeServerConfig
    method
  • Pass env, args, and headers to credential detection service
+6/-1     
mcp-config-service.types.ts
Add headers support and source tracking to type definitions

src/types/mcp-config-service.types.ts

  • Add headers field to MCPServerConfig and MCPServerInfo interfaces
  • Add source field to CredentialVariable interface with
    'env'|'args'|'headers' types
+3/-0     
Documentation
credential-detection.md
Update documentation for enhanced multi-source credential detection

docs/credential-detection.md

  • Update documentation to cover multi-source credential detection
  • Add examples for command arguments and HTTP headers detection
  • Document variable substitution safety features
  • Add comprehensive usage examples and security features
+176/-97

@lirantal lirantal self-assigned this Sep 27, 2025
@lirantal lirantal added the enhancement New feature or request label Sep 27, 2025
@github-actions github-actions bot added the documentation Improvements or additions to documentation label Sep 27, 2025
@qodo-code-review
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Pattern Scope

Header high-risk patterns include very broad matches like /auth/i and /x-.*-key/i which may over-flag benign headers (e.g., Proxy-Authorization, X-Api-Key-Name). Validate if these should be narrowed to reduce false positives or add allowlist logic.

high: [
  // Authorization headers
  /authorization/i,
  /auth/i,

  // API key headers
  /x-api-key/i,
  /api-key/i,
  /apikey/i,

  // Token headers
  /x-access-token/i,
  /access-token/i,
  /bearer-token/i,
  /x-auth-token/i,
  /auth-token/i,

  // Custom authentication headers
  /x-.*-key/i,
  /x-.*-token/i,
  /x-.*-secret/i,

  // Service-specific patterns
  /x-github-token/i,
  /x-openai-key/i,
  /x-anthropic-key/i
],
Arg Parsing

analyzeArguments assumes flag-value pairs only and ignores forms like --token=abc, short flags -t abc, or quoted values; consider supporting common variants to avoid missed detections.

static analyzeArguments(args: string[] | undefined): CredentialAnalysisResult {
  if (!args || !Array.isArray(args) || args.length === 0) {
    return {
      hasCredentials: false,
      credentialVars: [],
      riskLevel: 'none'
    }
  }

  const credentialVars: CredentialVariable[] = []

  // Check each argument and the next one for credential patterns
  for (let i = 0; i < args.length - 1; i++) {
    const arg = args[i]
    const nextArg = args[i + 1]

    // Check if current argument matches a credential flag pattern
    const isHighRiskArg = this.ARGUMENT_PATTERNS.high.some(pattern => pattern.test(arg))
    const isLowRiskArg = this.ARGUMENT_PATTERNS.low.some(pattern => pattern.test(arg))

    if (isHighRiskArg || isLowRiskArg) {
      // The next argument should be the value
      if (nextArg && !nextArg.startsWith('-')) {
        credentialVars.push({
          name: arg,
          value: this.maskValue(nextArg),
          riskLevel: isHighRiskArg ? 'high' : 'low',
          source: 'args'
        })
        i++ // Skip the next argument since we've processed it as a value
      }
    }
  }

  const hasCredentials = credentialVars.length > 0
  const riskLevel = this.calculateOverallRiskLevel(credentialVars)

  return {
    hasCredentials,
    credentialVars,
    riskLevel
  }
}
Output Size

Grouped credential summary prints full masked values and all entries inline; for many creds this may overflow CLI width. Consider truncation or per-source counts with optional expansion.

  // Create summary with source grouping
  const sourceSummaries = Object.entries(credsBySource).map(([source, creds]) => {
    const sourceIcon = getSourceIcon(source)
    const credList = creds.map(cv => `${cv.name}=${cv.value}`).join(', ')
    return `${sourceIcon}${source}: ${credList}`
  })

  const credentialSummary = sourceSummaries.join(' | ')

  return `${riskText} ${styleText(['dim'], `(${credentialVars.length} creds: ${credentialSummary})`)}`
}

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Sep 27, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Argument parsing logic is incomplete

Update the argument parsing logic in analyzeArguments to support both
space-separated (--key value) and equals-separated (--key=value) credential
formats for more comprehensive detection.

Examples:

src/services/credential-detection-service.ts [126-168]
  static analyzeArguments(args: string[] | undefined): CredentialAnalysisResult {
    if (!args || !Array.isArray(args) || args.length === 0) {
      return {
        hasCredentials: false,
        credentialVars: [],
        riskLevel: 'none'
      }
    }

    const credentialVars: CredentialVariable[] = []

 ... (clipped 33 lines)

Solution Walkthrough:

Before:

function analyzeArguments(args) {
  const credentials = [];
  // Loop only handles space-separated key-value pairs
  for (let i = 0; i < args.length - 1; i++) {
    const flag = args[i];
    const value = args[i + 1];

    if (isCredentialFlag(flag) && !value.startsWith('-')) {
      credentials.push({ name: flag, value: value });
      i++; // Skip the value argument
    }
  }
  return credentials;
}

After:

function analyzeArguments(args) {
  const credentials = [];
  for (let i = 0; i < args.length; i++) {
    const arg = args[i];

    if (arg.includes('=')) {
      const [flag, value] = arg.split('=', 2);
      if (isCredentialFlag(flag) && value) {
        credentials.push({ name: flag, value: value });
      }
    } else if (isCredentialFlag(arg) && args[i + 1] && !args[i + 1].startsWith('-')) {
      const flag = arg;
      const value = args[i + 1];
      credentials.push({ name: flag, value: value });
      i++; // Skip the value argument
    }
  }
  return credentials;
}
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a significant gap in the new argument parsing logic, as it fails to detect a common format (--key=value), leading to missed credential warnings.

Medium
Possible issue
Add variable substitution check for arguments

In analyzeArguments, add a check to ensure an argument's value is not a safe
variable substitution pattern before flagging it as a credential.

src/services/credential-detection-service.ts [146-157]

 if (isHighRiskArg || isLowRiskArg) {
-  // The next argument should be the value
-  if (nextArg && !nextArg.startsWith('-')) {
+  // The next argument should be the value, and not a variable substitution
+  if (nextArg && !nextArg.startsWith('-') && !this.isVariableSubstitution(nextArg)) {
     credentialVars.push({
       name: arg,
       value: this.maskValue(nextArg),
       riskLevel: isHighRiskArg ? 'high' : 'low',
       source: 'args'
     })
     i++ // Skip the next argument since we've processed it as a value
   }
 }
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug where analyzeArguments fails to check for safe variable substitutions, unlike analyzeHeaders and analyzeEnvironmentVariables, leading to inconsistent behavior and false positives.

Medium
  • Update

Signed-off-by: Liran Tal <liran.tal@gmail.com>
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.46154% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.01%. Comparing base (ae39afb) to head (7df583b).

Files with missing lines Patch % Lines
src/components/credential-warning.ts 14.28% 30 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
+ Coverage   77.67%   79.01%   +1.33%     
==========================================
  Files          19       19              
  Lines        2101     2354     +253     
  Branches      231      250      +19     
==========================================
+ Hits         1632     1860     +228     
- Misses        463      488      +25     
  Partials        6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Sep 27, 2025

Open in StackBlitz

npm i https://pkg.pr.new/lirantal/ls-mcp@98

commit: 7df583b

@lirantal lirantal merged commit 808d4ec into main Sep 27, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request Review effort 3/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants