Revert "Merge pull request #3048 from code-yeongyu/fix/p0-2-https-enforcement-gaps"
This reverts commitede561cb74, reversing changes made to2d13e125bb.
This commit is contained in:
@@ -41,7 +41,7 @@ describe("executeHttpHook TLS security", () => {
|
|||||||
const result = await executeHttpHook(hook, "{}")
|
const result = await executeHttpHook(hook, "{}")
|
||||||
|
|
||||||
expect(result.exitCode).toBe(1)
|
expect(result.exitCode).toBe(1)
|
||||||
expect(result.stderr).toContain("HTTP hook URL must use HTTPS")
|
expect(result.stderr).toContain("HTTP hook URL must use HTTPS in production")
|
||||||
expect(mockFetch).not.toHaveBeenCalled()
|
expect(mockFetch).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
|
|
||||||
@@ -52,7 +52,7 @@ describe("executeHttpHook TLS security", () => {
|
|||||||
const result = await executeHttpHook(hook, "{}")
|
const result = await executeHttpHook(hook, "{}")
|
||||||
|
|
||||||
expect(result.exitCode).toBe(1)
|
expect(result.exitCode).toBe(1)
|
||||||
expect(result.stderr).toContain("HTTP hook URL must use HTTPS")
|
expect(result.stderr).toContain("HTTP hook URL must use HTTPS in production")
|
||||||
expect(mockFetch).not.toHaveBeenCalled()
|
expect(mockFetch).not.toHaveBeenCalled()
|
||||||
})
|
})
|
||||||
|
|
||||||
@@ -108,15 +108,14 @@ describe("executeHttpHook TLS security", () => {
|
|||||||
process.env = { ...originalEnv, NODE_ENV: "development" }
|
process.env = { ...originalEnv, NODE_ENV: "development" }
|
||||||
})
|
})
|
||||||
|
|
||||||
it("#when hook uses remote http:// URL #then rejects with exit code 1", async () => {
|
it("#when hook uses remote http:// URL #then allows execution", async () => {
|
||||||
const { executeHttpHook } = await import("./execute-http-hook")
|
const { executeHttpHook } = await import("./execute-http-hook")
|
||||||
const hook: HookHttp = { type: "http", url: "http://example.com/hooks" }
|
const hook: HookHttp = { type: "http", url: "http://example.com/hooks" }
|
||||||
|
|
||||||
const result = await executeHttpHook(hook, "{}")
|
const result = await executeHttpHook(hook, "{}")
|
||||||
|
|
||||||
expect(result.exitCode).toBe(1)
|
expect(result.exitCode).toBe(0)
|
||||||
expect(result.stderr).toContain("HTTP hook URL must use HTTPS")
|
expect(mockFetch).toHaveBeenCalledTimes(1)
|
||||||
expect(mockFetch).not.toHaveBeenCalled()
|
|
||||||
})
|
})
|
||||||
|
|
||||||
it("#when hook uses http://localhost #then allows execution", async () => {
|
it("#when hook uses http://localhost #then allows execution", async () => {
|
||||||
@@ -152,59 +151,6 @@ describe("executeHttpHook TLS security", () => {
|
|||||||
url: "http://example.com/hooks",
|
url: "http://example.com/hooks",
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|
||||||
it("#when hook uses http://[::1] #then allows execution", async () => {
|
|
||||||
const { executeHttpHook } = await import("./execute-http-hook")
|
|
||||||
const hook: HookHttp = { type: "http", url: "http://[::1]:8080/hooks" }
|
|
||||||
|
|
||||||
const result = await executeHttpHook(hook, "{}")
|
|
||||||
|
|
||||||
expect(result.exitCode).toBe(0)
|
|
||||||
expect(mockFetch).toHaveBeenCalledTimes(1)
|
|
||||||
})
|
|
||||||
})
|
|
||||||
|
|
||||||
describe("#given NODE_ENV is unset", () => {
|
|
||||||
beforeEach(() => {
|
|
||||||
process.env = { ...originalEnv }
|
|
||||||
delete process.env.NODE_ENV
|
|
||||||
})
|
|
||||||
|
|
||||||
it("#when hook uses remote http:// URL #then rejects with exit code 1", async () => {
|
|
||||||
const { executeHttpHook } = await import("./execute-http-hook")
|
|
||||||
const hook: HookHttp = { type: "http", url: "http://example.com/hooks" }
|
|
||||||
|
|
||||||
const result = await executeHttpHook(hook, "{}")
|
|
||||||
|
|
||||||
expect(result.exitCode).toBe(1)
|
|
||||||
expect(result.stderr).toContain("HTTP hook URL must use HTTPS")
|
|
||||||
expect(mockFetch).not.toHaveBeenCalled()
|
|
||||||
})
|
|
||||||
})
|
|
||||||
|
|
||||||
describe("#given redirect downgrade protection", () => {
|
|
||||||
beforeEach(() => {
|
|
||||||
process.env = { ...originalEnv, NODE_ENV: "production" }
|
|
||||||
})
|
|
||||||
|
|
||||||
it("#when hook uses https:// URL #then fetch uses manual redirect handling", async () => {
|
|
||||||
mockFetch.mockImplementation(() =>
|
|
||||||
Promise.resolve(new Response("redirect", { status: 302, statusText: "Found" }))
|
|
||||||
)
|
|
||||||
const { executeHttpHook } = await import("./execute-http-hook")
|
|
||||||
const hook: HookHttp = { type: "http", url: "https://example.com/hooks" }
|
|
||||||
|
|
||||||
const result = await executeHttpHook(hook, "{}")
|
|
||||||
|
|
||||||
expect(result.exitCode).toBe(1)
|
|
||||||
expect(result.stderr).toContain("HTTP hook returned status 302")
|
|
||||||
expect(mockFetch).toHaveBeenCalledWith(
|
|
||||||
"https://example.com/hooks",
|
|
||||||
expect.objectContaining({
|
|
||||||
redirect: "manual",
|
|
||||||
})
|
|
||||||
)
|
|
||||||
})
|
|
||||||
})
|
})
|
||||||
|
|
||||||
describe("#given invalid URL handling is preserved", () => {
|
describe("#given invalid URL handling is preserved", () => {
|
||||||
|
|||||||
@@ -4,10 +4,13 @@ import { log } from "../../shared"
|
|||||||
|
|
||||||
const DEFAULT_HTTP_HOOK_TIMEOUT_S = 30
|
const DEFAULT_HTTP_HOOK_TIMEOUT_S = 30
|
||||||
const ALLOWED_SCHEMES = new Set(["http:", "https:"])
|
const ALLOWED_SCHEMES = new Set(["http:", "https:"])
|
||||||
const LOCALHOST_HOSTNAMES = new Set(["localhost", "127.0.0.1", "::1", "[::1]"])
|
|
||||||
|
function isProduction(): boolean {
|
||||||
|
return process.env.NODE_ENV === "production"
|
||||||
|
}
|
||||||
|
|
||||||
function isLocalhost(url: URL): boolean {
|
function isLocalhost(url: URL): boolean {
|
||||||
return LOCALHOST_HOSTNAMES.has(url.hostname)
|
return url.hostname === "localhost" || url.hostname === "127.0.0.1"
|
||||||
}
|
}
|
||||||
|
|
||||||
function isPlainHttp(url: URL): boolean {
|
function isPlainHttp(url: URL): boolean {
|
||||||
@@ -65,10 +68,10 @@ export async function executeHttpHook(
|
|||||||
|
|
||||||
if (isPlainHttp(parsed)) {
|
if (isPlainHttp(parsed)) {
|
||||||
log("HTTP hook URL uses insecure protocol", { url: hook.url })
|
log("HTTP hook URL uses insecure protocol", { url: hook.url })
|
||||||
if (!isLocalhost(parsed)) {
|
if (isProduction() && !isLocalhost(parsed)) {
|
||||||
return {
|
return {
|
||||||
exitCode: 1,
|
exitCode: 1,
|
||||||
stderr: "HTTP hook URL must use HTTPS. Plain HTTP is only allowed for localhost, 127.0.0.1, and ::1.",
|
stderr: "HTTP hook URL must use HTTPS in production. Plain HTTP is only allowed for localhost/127.0.0.1.",
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -81,7 +84,6 @@ export async function executeHttpHook(
|
|||||||
method: "POST",
|
method: "POST",
|
||||||
headers,
|
headers,
|
||||||
body: stdin,
|
body: stdin,
|
||||||
redirect: "manual",
|
|
||||||
signal: AbortSignal.timeout(timeoutS * 1000),
|
signal: AbortSignal.timeout(timeoutS * 1000),
|
||||||
})
|
})
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user