From e3bfec354e56499a2266a45804d0a93d17f46bce Mon Sep 17 00:00:00 2001 From: Casey Marshall Date: Wed, 26 Jun 2024 13:10:16 -0500 Subject: [PATCH] fix: align auth failure error messages for oauth In the Typescript CLI, when OAuth fails due to an invalid or expired token, throw the same "missing auth" error that one would get if not logged in (no auth token). The resulting error message will prompt the user to re-authenticate. This has to be mediated by the legacycli http proxy because the Typescript side of the CLI is not involved in the OAuth flow at all; it is handled transparently by the go-application-framework's networking middleware. A special HTTP request and response header is used by the legacycli proxy to indicate whether that middleware auth has failed. If it has, the functions which perform HTTP client requests with needle will throw an error that indicates "unauthenticated", resulting in an experience consistent with the legacy token auth flow in the CLI. CLI-392 --- cliv2/internal/proxy/proxy.go | 31 ++++++++++++++- cliv2/internal/proxy/proxy_test.go | 37 ++++++++++++++++++ src/lib/ecosystems/resolve-test-facts.ts | 11 ++++-- src/lib/errors/missing-api-token.ts | 11 ++++++ src/lib/request/constants.ts | 1 + src/lib/request/promise.ts | 5 +++ src/lib/request/request.ts | 10 ++++- .../resolve-test-facts.oauth.spec.ts | 39 +++++++++++++++++++ test/jest/unit/lib/request/request.spec.ts | 30 ++++++++++++++ 9 files changed, 169 insertions(+), 6 deletions(-) create mode 100644 src/lib/request/constants.ts create mode 100644 test/jest/unit/lib/ecosystems/resolve-test-facts.oauth.spec.ts create mode 100644 test/jest/unit/lib/request/request.spec.ts 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.', + }), + ); + }); +});