diff --git a/cliv2/internal/proxy/proxy.go b/cliv2/internal/proxy/proxy.go index 0395cc6f32..ca27f42a2c 100644 --- a/cliv2/internal/proxy/proxy.go +++ b/cliv2/internal/proxy/proxy.go @@ -4,6 +4,7 @@ import ( "context" "crypto/tls" "crypto/x509" + "errors" "fmt" "log" "net" @@ -18,6 +19,7 @@ import ( pkg_utils "github.com/snyk/go-application-framework/pkg/utils" "github.com/snyk/go-application-framework/pkg/networking/certs" + "github.com/snyk/go-application-framework/pkg/networking/middleware" "github.com/snyk/go-httpauth/pkg/httpauth" "github.com/snyk/cli/cliv2/internal/constants" @@ -143,9 +145,28 @@ func (p *WrapperProxy) ProxyInfo() *ProxyInfo { } } +// headerSnykAuthFailed is used to indicate there was a failure to establish +// authorization in a legacycli proxied HTTP request and response. +// +// The request header is used to propagate this indication from +// NetworkAccess.AddHeaders all the way through proxy middleware into the +// response. +// +// The response header is then used by the Typescript CLI to surface an +// appropriate authentication failure error back to the user. +// +// These layers of indirection are necessary because the Typescript CLI is not +// involved in OAuth authentication at all, but needs to know that an auth +// failure specifically occurred. HTTP status and error catalog codes aren't +// adequate for this purpose because there are non-authentication reasons an API +// request might 401 or 403, such as permissions or entitlements. +const headerSnykAuthFailed = "snyk-auth-failed" + func (p *WrapperProxy) replaceVersionHandler(r *http.Request, ctx *goproxy.ProxyCtx) (*http.Request, *http.Response) { - err := p.addHeaderFunc(r) - if err != nil { + if err := p.addHeaderFunc(r); err != nil { + if errors.Is(err, middleware.ErrAuthenticationFailed) { + r.Header.Set(headerSnykAuthFailed, "true") + } p.DebugLogger.Printf("Failed to add header: %s", err) } @@ -177,6 +198,12 @@ func (p *WrapperProxy) Start() error { proxy.Logger = log.New(&pkg_utils.ToZeroLogDebug{Logger: p.DebugLogger}, "", 0) proxy.OnRequest().DoFunc(p.replaceVersionHandler) proxy.OnRequest().HandleConnect(p) + proxy.OnResponse().DoFunc(func(resp *http.Response, ctx *goproxy.ProxyCtx) *http.Response { + if authFailed := resp.Request.Header.Get(headerSnykAuthFailed); authFailed != "" { + resp.Header.Set(headerSnykAuthFailed, authFailed) + } + return resp + }) proxy.Verbose = true proxyServer := &http.Server{ Handler: proxy, diff --git a/cliv2/internal/proxy/proxy_test.go b/cliv2/internal/proxy/proxy_test.go index 359f954fcc..e8b68479e7 100644 --- a/cliv2/internal/proxy/proxy_test.go +++ b/cliv2/internal/proxy/proxy_test.go @@ -13,6 +13,7 @@ import ( "github.com/rs/zerolog" "github.com/snyk/go-application-framework/pkg/configuration" "github.com/snyk/go-application-framework/pkg/networking/certs" + "github.com/snyk/go-application-framework/pkg/networking/middleware" gafUtils "github.com/snyk/go-application-framework/pkg/utils" "github.com/snyk/go-httpauth/pkg/httpauth" @@ -256,3 +257,39 @@ func Test_appendExtraCaCert(t *testing.T) { // cleanup os.Remove(file.Name()) } + +func Test_proxyPropagatesAuthFailureHeader(t *testing.T) { + basecache := "testcache" + version := "1.1.1" + config := configuration.NewInMemory() + config.Set(configuration.CACHE_PATH, basecache) + config.Set(configuration.INSECURE_HTTPS, false) + + setup(t, basecache, version) + defer teardown(t, basecache) + + wp, err := proxy.NewWrapperProxy(config, version, &debugLogger) + assert.Nil(t, err) + wp.SetHeaderFunction(func(r *http.Request) error { + // Simulate a wrapped authentication failure, such as oauth refresh. + return fmt.Errorf("nope: %w", middleware.ErrAuthenticationFailed) + }) + + err = wp.Start() + assert.Nil(t, err) + + useProxyAuth := true + proxiedClient, err := helper_getHttpClient(wp, useProxyAuth) + assert.Nil(t, err) + + res, err := proxiedClient.Get("https://static.snyk.io/cli/latest/version") + assert.Nil(t, err) + // Assert that the proxy propagates the auth failed marker header to the response. + assert.Equal(t, res.Header.Get("snyk-auth-failed"), "true") + + wp.Close() + + // assert cert file is deleted on Close + _, err = os.Stat(wp.CertificateLocation) + assert.NotNil(t, err) // this means the file is gone +} diff --git a/src/lib/ecosystems/resolve-test-facts.ts b/src/lib/ecosystems/resolve-test-facts.ts index 4be6a1c77b..85eb24ef67 100644 --- a/src/lib/ecosystems/resolve-test-facts.ts +++ b/src/lib/ecosystems/resolve-test-facts.ts @@ -43,9 +43,14 @@ export async function resolveAndTestFacts( try { return await resolveAndTestFactsUnmanagedDeps(scans, options); } catch (error) { - const unauthorized = error.code === 401 || error.code === 403; - - if (unauthorized) { + const unauthorizedErrorCode = error.code === 401 || error.code === 403; + const missingApiToken = error.isMissingApiToken; + + // Decide if the error is an authorization error other than being + // unauthenticated (missing or invalid API token). An account lacking + // permission, for example. + const otherUnauthorized = unauthorizedErrorCode && !missingApiToken; + if (otherUnauthorized) { throw AuthFailedError( 'Unauthorized request to unmanaged service', error.code, diff --git a/src/lib/errors/missing-api-token.ts b/src/lib/errors/missing-api-token.ts index 0e6a28fc8a..370d2fedff 100644 --- a/src/lib/errors/missing-api-token.ts +++ b/src/lib/errors/missing-api-token.ts @@ -6,6 +6,17 @@ export class MissingApiTokenError extends CustomError { private static ERROR_MESSAGE = '`snyk` requires an authenticated account. Please run `snyk auth` and try again.'; + /** + * isMissingApiToken returns whether the error instance is a missing API token + * error. + * + * Defined as a property so that the same expression resolves as "falsy" + * (undefined) when other error types are tested. + */ + public get isMissingApiToken(): boolean { + return this.strCode === MissingApiTokenError.ERROR_STRING_CODE; + } + constructor() { super(MissingApiTokenError.ERROR_MESSAGE); this.code = MissingApiTokenError.ERROR_CODE; diff --git a/src/lib/request/constants.ts b/src/lib/request/constants.ts new file mode 100644 index 0000000000..04a4521006 --- /dev/null +++ b/src/lib/request/constants.ts @@ -0,0 +1 @@ +export const headerSnykAuthFailed = 'snyk-auth-failed'; diff --git a/src/lib/request/promise.ts b/src/lib/request/promise.ts index 48e1cea59c..f15b9b5bdf 100644 --- a/src/lib/request/promise.ts +++ b/src/lib/request/promise.ts @@ -1,4 +1,6 @@ import { getAuthHeader } from '../api-token'; +import { MissingApiTokenError } from '../errors'; +import { headerSnykAuthFailed } from './constants'; import * as request from './index'; export async function makeRequest(payload: any): Promise { @@ -36,6 +38,9 @@ export async function makeRequestRest(payload: any): Promise { if (error) { return reject(error); } + if (res?.headers?.[headerSnykAuthFailed] === 'true') { + return reject(new MissingApiTokenError()); + } if (res.statusCode === 400) { return reject({ code: res.statusCode, diff --git a/src/lib/request/request.ts b/src/lib/request/request.ts index c09617be75..306cc8ff81 100644 --- a/src/lib/request/request.ts +++ b/src/lib/request/request.ts @@ -12,6 +12,8 @@ import { getVersion } from '../version'; import * as https from 'https'; import * as http from 'http'; import { jsonStringifyLargeObject } from '../json'; +import { MissingApiTokenError } from '../errors'; +import { headerSnykAuthFailed } from './constants'; const debug = debugModule('snyk:req'); const snykDebug = debugModule('snyk'); @@ -143,6 +145,9 @@ export async function makeRequest( return new Promise((resolve, reject) => { needle.request(method, url, data, options, (err, res, respBody) => { + if (res?.headers?.[headerSnykAuthFailed] === 'true') { + return reject(new MissingApiTokenError()); + } // respBody potentially very large, do not output it in debug debug('response (%s)', (res || {}).statusCode); if (err) { @@ -173,7 +178,10 @@ export async function streamRequest( async function getStatusCode(stream: needle.ReadableStream): Promise { return new Promise((resolve, reject) => { - stream.on('header', (statusCode: number) => { + stream.on('header', (statusCode: number, headers: any) => { + if (headers?.[headerSnykAuthFailed] === 'true') { + return reject(new MissingApiTokenError()); + } resolve(statusCode); }); stream.on('err', (err: Error) => { diff --git a/test/jest/unit/lib/ecosystems/resolve-test-facts.oauth.spec.ts b/test/jest/unit/lib/ecosystems/resolve-test-facts.oauth.spec.ts new file mode 100644 index 0000000000..88a8f2ab00 --- /dev/null +++ b/test/jest/unit/lib/ecosystems/resolve-test-facts.oauth.spec.ts @@ -0,0 +1,39 @@ +const mockMakeRequest = jest.fn(); + +import { Options } from '../../../../../src/lib/types'; +import { MissingApiTokenError } from '../../../../../src/lib/errors'; + +import { resolveAndTestFacts } from '../../../../../src/lib/ecosystems/resolve-test-facts'; + +jest.mock('../../../../../src/lib/request/request', () => { + return { + makeRequest: mockMakeRequest, + }; +}); + +describe('oauth failure', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + it('rethrows same error for missing api token', async () => { + mockMakeRequest.mockRejectedValue(new MissingApiTokenError()); + await expect(resolveAndTestFacts('cpp', {}, {} as Options)).rejects.toThrow( + expect.objectContaining({ + message: + '`snyk` requires an authenticated account. Please run `snyk auth` and try again.', + }), + ); + }); + + it('rethrows general error for other api auth failures', async () => { + const err: any = new Error('nope'); + err.code = 403; + mockMakeRequest.mockRejectedValue(err); + await expect(resolveAndTestFacts('cpp', {}, {} as Options)).rejects.toThrow( + expect.objectContaining({ + message: 'Unauthorized request to unmanaged service', + }), + ); + }); +}); diff --git a/test/jest/unit/lib/request/request.spec.ts b/test/jest/unit/lib/request/request.spec.ts new file mode 100644 index 0000000000..61a21c6f19 --- /dev/null +++ b/test/jest/unit/lib/request/request.spec.ts @@ -0,0 +1,30 @@ +const mockNeedleRequest = jest.fn(); + +import { makeRequest } from '../../../../../src/lib/request/request'; +import { Payload } from '../../../../../src/lib/request/types'; + +jest.mock('needle', () => { + return { + request: mockNeedleRequest, + }; +}); + +describe('needle header auth failed', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + it('throws missing api token on auth failed marker header', async () => { + mockNeedleRequest.mockImplementation((method, url, data, options, fn) => { + fn(null, { headers: { 'snyk-auth-failed': 'true' } }, {}); + }); + await expect( + makeRequest({ url: 'https://example.com' } as Payload), + ).rejects.toThrow( + expect.objectContaining({ + message: + '`snyk` requires an authenticated account. Please run `snyk auth` and try again.', + }), + ); + }); +});