googleapis / googleapis/google-cloud-node

google-auth-library: getErrorFromOAuthErrorResponse() copies stack as non-writable, breaking error decoration in consumers

Đang mở Phù hợp với người mới
#9,155 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
TypeScript
Star
3.2k
Fork
712
Merge trung bình
2 ngày 9 giờ
Pull request đã merge (30 ngày)
104

Mô tả

### Environment

- `google-auth-library`: **11.0.2** (also reproduced on 10.6.2)
- Node.js: 20.19.5
- Credential type: `external_account` (Workload Identity Federation, file-sourced OIDC subject token)

### Summary

`getErrorFromOAuthErrorResponse()` — [`core/packages/google-auth-library-nodejs/src/auth/oauth2common.ts:253`](https://github.com/googleapis/google-cloud-node/blob/main/core/packages/google-auth-library-nodejs/src/auth/oauth2common.ts#L253) — builds a new `Error` from an OAuth/STS error response and copies the original error's own properties onto it — including `stack` — as **non-writable**:

```js
const keys = Object.keys(err);
if (err.stack) {
// Copy error.stack if available.
keys.push('stack');
}
keys.forEach(key => {
if (key !== 'message') {
Object.defineProperty(newError, key, {
value: err[key],
writable: false, // <-- makes `stack` read-only
enumerable: true,
});
}
});
```

Every error surfaced from an STS/OAuth error response therefore has `stack` with `{ writable: false, enumerable: true, configurable: true }`, whereas a normal `Error` has a writable `stack`.

### Why it matters

Appending to `error.stack` to attach causal context is a common pattern, and it is normally safe. Against these errors it throws in strict mode (and silently discards the write in sloppy mode). Both outcomes are bad, and the strict-mode one is worse: the real authentication failure is replaced by a `TypeError` naming an unrelated library.

A concrete case in the same ecosystem — `@google-cloud/firestore`'s `wrapError()` (`build/src/util.js`) does:

```js
err.stack += '\nCaused by: ' + stack;
```

Its module is emitted with `"use strict"`, and it is invoked from stream `'error'` handlers. So when a Firestore operation fails because a federated credential was rejected, the `TypeError` is thrown inside an `EventEmitter` emit, escapes the promise chain, and becomes an **`uncaughtException`** rather than a rejected promise:

```
TypeError: Cannot assign to read only property 'stack' of object 'Error: Error code invalid_grant: ID Token issued at is stale to sign-in.'
at wrapError (/app/node_modules/@google-cloud/firestore/build/src/util.js:213:15)
at Transform.emit (node:events:524:28)
at emitErrorNT (node:internal/streams/destroy:169:8)
at process.processTicksAndRejections (node:internal/process/task_queues:82:21)
```

The underlying problem was an expired OIDC subject token. Nothing in what the process reported says so. I've filed the counterpart issue for the Firestore half (same monorepo) asking them to guard the assignment: #9154. But the read-only `stack` looks worth revisiting here too, since any consumer decorating errors this way will hit it, and `stack` is conventionally writable.

Also note `enumerable: true` on the copied properties: `stack` is normally non-enumerable, so this additionally makes it show up in `Object.keys(err)`, `for...in`, and `JSON.stringify` output of the error's own properties.

### Reproduction

```js
// npm i google-auth-library
const { getErrorFromOAuthErrorResponse } = require('google-auth-library/build/src/auth/oauth2common.js');

const authError = getErrorFromOAuthErrorResponse(
{ error: 'invalid_grant', error_description: 'ID Token is stale to sign-in.' },
new Error('underlying transport failure'),
);

console.log(Object.getOwnPropertyDescriptor(authError, 'stack'));
// => { value: '...', writable: false, enumerable: true, configurable: true }

// Sloppy mode: the write is silently discarded.
(function () { authError.stack += '\nCaused by: x'; })();

// Strict mode (what compiled TS/ESM consumers run in): throws.
(function () { 'use strict'; authError.stack += '\nCaused by: x'; })();
// => TypeError: Cannot assign to read only property 'stack' of object
// 'Error: Error code invalid_grant: ID Token is stale to sign-in.'
```

In situ this is reached via a normal `external_account` credential whose subject token has expired: the STS response is an `invalid_grant`, `getErrorFromOAuthErrorResponse()` converts it, and any downstream `err.stack +=` then crashes.

### Suggested fix

Leave `stack` writable (and non-enumerable) when copying, so these errors behave like ordinary ones:

```js
keys.forEach(key => {
if (key !== 'message') {
Object.defineProperty(newError, key, {
value: err[key],
writable: key === 'stack',
enumerable: key !== 'stack',
configurable: true,
});
}
});
```

Simply not copying `stack` at all would also work — `newError` already has its own accurate stack — though that loses the original call site. Alternatively `newError.cause = err` conveys the same information using the standard mechanism and keeps `stack` untouched.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Bắt đầu trong core/packages/google-auth-library-nodejs/src/auth/oauth2common.ts tại getErrorFromOAuthErrorResponse(), sau đó chạy quá trình tái hiện với một lỗi OAuth/STS của external_account. Thay đổi được hoàn tất khi lỗi tạo ra vẫn giữ stack ở trạng thái có thể ghi và không thể liệt kê, đồng thời bảo toàn thông tin lỗi mong muốn, và việc trang trí stack trong strict mode không còn gây ra ngoại lệ.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
nodejs, typescript
Lĩnh vực
authentication
Loại issue
Lỗi
Độ khó
2/5
Thời gian dự kiến
1-3 giờ
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Đặc tả rõ ràng
Mức phù hợp với người mới
74/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.