Skip to content

Cap redirects in bin/install.js fetchRawText to prevent loop #69

@derek-palmer

Description

@derek-palmer

This was generated by AI during triage.

Problem Statement

fetchRawText(url) in bin/install.js follows HTTP 301/302 redirects by recursing into get(res.headers.location) with no depth limit. A redirect cycle (301 → 301 → …) or a long redirect chain causes unbounded recursion and request fan-out before any failure surfaces. Pre-existing (predates #65; the loader work only renamed get(url)fetchRawText and moved it out of fetchSkill).

Real-world risk is low: the only caller URLs target RAW_BASE (raw.githubusercontent.com, trusted), reached only when no local checkout/packaged tasks.json or SKILL.md exists. But the function is now shared by fetchSkill and loadTaskSkillSlugs, so hardening it protects both.

Solution

Add a max-redirect cap. On exceeding it, resolve null (same failure contract the function already uses for non-200/error/timeout).

Implementation Decisions

  • Thread a depth counter (or remaining-redirect budget) through the inner get(); default cap 5.
  • On cap exceeded: res.resume() to drain, then resolve(null).
  • Keep the existing null-on-failure contract — callers already treat null as "fetch failed".
  • Guard against a missing location header (resolve null rather than get(undefined)).

Testing Decisions

  • Add a node --test case in tests/install.test.js using a stub https.get that always returns a 301 to itself; assert the promise resolves null and does not hang/overflow.
  • Add a case for a finite redirect chain within the cap resolving the body.

Out of Scope

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions