-
-
Notifications
You must be signed in to change notification settings - Fork 916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Throw helpful error messages #1126
Comments
https://github.com/sindresorhus/got/blob/master/readme.md#errors It's your responsibility to catch the error and read the useful information. Options aren't logged on purpose to prevent the leak of tokens. @sindresorhus I think each error message should contain the request method and its URL. |
Agreed |
FWIW, I had the same problem as @kirillgroshkov today and would have found the proposed functionality useful and intuitive |
Encountered this today. Error is thrown and no useful information can be derived from the error thrown. Not even the body returned from the request. |
I agree too. I'm starting to use the lib and that was my first unconfortable usecase. Would be awesome to receive formated erros by default. |
The problem is we can't identify what kind of actual errors are really sent from the API, for example, 400 Bad Request could mean an invalid token but it could be mean something else too. |
HTTPError: Response code 403 (Forbidden)
at Request.<anonymous> (node_modules/got/dist/source/as-promise/index.js:117:42)
at processTicksAndRejections (internal/process/task_queues.js:93:5) {
code: undefined,
timings: {
start: 1613304139851,
socket: 1613304139851,
lookup: 1613304139851,
connect: 1613304139917,
secureConnect: 1613304139986,
upload: 1613304139986,
response: 1613304140680,
end: 1613304140681,
error: undefined,
abort: undefined,
phases: {
wait: 0,
dns: 0,
tcp: 66,
tls: 69,
request: 0,
firstByte: 694,
download: 1,
total: 830
}
}
} |
In Got v12, click hereHTTPError: Response code 404 (NOT FOUND)
at Request.<anonymous> (file:///home/szm/Desktop/got/dist/source/as-promise/index.js:89:42)
at Object.onceWrapper (node:events:485:26)
at Request.emit (node:events:390:22)
at Request.EventEmitter.emit (node:domain:532:15)
at Request._onResponseBase (file:///home/szm/Desktop/got/dist/source/core/index.js:716:22)
at processTicksAndRejections (node:internal/process/task_queues:94:5)
at async Request._onResponse (file:///home/szm/Desktop/got/dist/source/core/index.js:755:13) {
input: undefined,
code: 'ERR_NON_2XX_3XX_RESPONSE',
timings: {
start: 1626356829477,
socket: 1626356829477,
lookup: 1626356829494,
connect: 1626356829627,
secureConnect: 1626356829763,
upload: 1626356829764,
response: 1626356829902,
end: 1626356829904,
error: undefined,
abort: undefined,
phases: {
wait: 0,
dns: 17,
tcp: 133,
tls: 136,
request: 1,
firstByte: 138,
download: 2,
total: 427
}
},
options: {
request: undefined,
agent: { http: undefined, https: undefined, http2: undefined },
h2session: undefined,
decompress: true,
timeout: {
connect: undefined,
lookup: undefined,
read: undefined,
request: undefined,
response: undefined,
secureConnect: undefined,
send: undefined,
socket: undefined
},
prefixUrl: '',
body: undefined,
form: undefined,
json: undefined,
cookieJar: undefined,
ignoreInvalidCookies: false,
searchParams: undefined,
dnsLookup: undefined,
dnsCache: undefined,
context: {},
hooks: {
init: [],
beforeRequest: [],
beforeError: [],
beforeRedirect: [],
beforeRetry: [],
afterResponse: []
},
followRedirect: true,
maxRedirects: 10,
cache: undefined,
throwHttpErrors: true,
username: '',
password: '',
http2: false,
allowGetBody: false,
headers: {
'user-agent': 'got (https://github.com/sindresorhus/got)',
accept: 'application/json',
'accept-encoding': 'gzip, deflate, br'
},
methodRewriting: false,
dnsLookupIpVersion: undefined,
parseJson: [Function: parse],
stringifyJson: [Function: stringify],
retry: {
limit: 2,
methods: [ 'GET', 'PUT', 'HEAD', 'DELETE', 'OPTIONS', 'TRACE' ],
statusCodes: [
408, 413, 429, 500,
502, 503, 504, 521,
522, 524
],
errorCodes: [
'ETIMEDOUT',
'ECONNRESET',
'EADDRINUSE',
'ECONNREFUSED',
'EPIPE',
'ENOTFOUND',
'ENETUNREACH',
'EAI_AGAIN'
],
maxRetryAfter: undefined,
calculateDelay: [Function: calculateDelay],
backoffLimit: Infinity,
noise: 100
},
localAddress: undefined,
method: 'GET',
createConnection: undefined,
cacheOptions: {
shared: undefined,
cacheHeuristic: undefined,
immutableMinTimeToLive: undefined,
ignoreCargoCult: undefined
},
httpsOptions: {
alpnProtocols: undefined,
rejectUnauthorized: undefined,
checkServerIdentity: undefined,
certificateAuthority: undefined,
key: undefined,
certificate: undefined,
passphrase: undefined,
pfx: undefined,
ciphers: undefined,
honorCipherOrder: undefined,
minVersion: undefined,
maxVersion: undefined,
signatureAlgorithms: undefined,
tlsSessionLifetime: undefined,
dhparam: undefined,
ecdhCurve: undefined,
certificateRevocationLists: undefined
},
encoding: undefined,
resolveBodyOnly: false,
isStream: false,
responseType: 'text',
url: URL {
href: 'https://httpbin.org/https://httpbin.org/status/404',
origin: 'https://httpbin.org',
protocol: 'https:',
username: '',
password: '',
host: 'httpbin.org',
hostname: 'httpbin.org',
port: '',
pathname: '/https://httpbin.org/status/404',
search: '',
searchParams: URLSearchParams {},
hash: ''
},
pagination: {
transform: [Function: transform],
paginate: [Function: paginate],
filter: [Function: filter],
shouldContinue: [Function: shouldContinue],
countLimit: Infinity,
backoff: 0,
requestLimit: 10000,
stackAllItems: false
},
setHost: true,
maxHeaderSize: undefined,
searchParameters: undefined
}
} We could attach @sindresorhus Any thoughts? |
No one has commented whether it's safe to expose the body on the error though. I'm not against doing it, but I am concerned it will later turn into a security vulnerability of some kind. Someone will have to do some research about it. One way to figure this out would be to look at other popular request libraries. For example, the Python one doesn't seem to expose the body directly on the error: https://stackoverflow.com/a/58085336/64949 There are multiple ways we could solve this:
|
It's already possible via the |
I think this is a sane choice. I would not worry so much about HTTP-client-specific security concerns here because the security problem clearly is in the HTTP server if that decides to write sensitive information into an HTTP response body. And then of course this should be treated as "bytes", not "characters". In the sense that it should totally be prepared for gibberish from the point of view of a certain text encoding -- it should be able to show arbitrary byte sequences. A good way to approach this would be to do utf-8 decoding with a fallback to showing non-decodable fragments, too. Something like >>> b'\x80abc'.decode("utf-8", "backslashreplace")
'\\x80abc' for Python. |
I disagree with that. The server may output sensitive info which is useful for debugging. E.g. GitHub hides secrets when running GitHub Actions. Got 12 exposes the entire Options object which should be enough to reproduce the issue and see for yourself. |
But only in development mode :) That's how the web application frameworks work that I used so far. In production mode, no sensitive data (such as error detail) is supposed to leak into the response body. But after all of course it's about the complete system, and the developer(s) responsible for both. I am just saying that I think it's fair to -- by default -- log parts of the response body upon error, I think it's of more use than harm; based on many years of experience. Often times, it's exactly the response body that's lacking when people run into an error for the first time and it may contain the critical detail to really understand what happened. |
Unfortunately the world isn't perfect :P
I agree. But still the security concern doesn't go away. We need more feedback. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@erickhavel This issue is about making errors more helpful. You seem to be asking something different, directly related to your work. Please read the actual docs, Got can return the response body on error status code as well. |
TL;DR The default behavior of got library should balance security and pragmatism and should be configurable by the user @szmarczak @sindresorhus I'm very happy to hear that error.options will become enumerable out of the box in got@12. I reported this a very long time ago here #1067 I'm curious what changed in the meantime. I'm afraid that making whole error.options enumerable it's going to be too verbose unless irrelevant/null/undefined properties are filtered out but it's definitely better than nothing. Regarding error.body in most cases servers responding with 4xx or 5xx do not include sensitive data but information about why the error occurred. For example Google Storage API returns information about missing permissions which is very helpful for debugging (service account myapp does not have permission storage.objects.write for bucket mybucket). On one side there is a security concern if
On the other side debugging every HTTP issue is taking minutes or hours instead of seconds OR you end up with a lot of boilerplate code OR you have to use a beforeError hook. None of those is preferable. I propose that at the very least a set of properties considered safe are enumerable by default:
Currently I end up with code like this because got errors are useless in an application that calls multiple endpoints. I can't even know which API call failed unless I use a beforeError hook or this:
I would like to see a sane default behavior in a future release of got. I would prefer having sensitive information in logs over writing code like this or spending time trying to reproduce or guess an error to see which API call failed every time it happens. |
For the record, I (the topic starter) "solved" it for myself by wrapping @aalexgabi, your last message very well described the experience that I had when opening this issue. |
This is on purpose. Copy n' paste those to reproduce the error on any Got instance. |
I agreed, Got Error are useless : we have not the response in the error message and the error is really big to log with useless informations. It's a pain to simplify and know what the error is |
I may have missed it in this thread, but is it still not possible to get the body of the response when there is an error (in the "catch" block)? For example:
thank you! |
I made this to change Got Error and get response error (I use got-cjs, not directly got) : import type { OptionsOfUnknownResponseBody, RequestError } from 'got-cjs';
import { got as originGot } from 'got-cjs';
interface GotErrorResponse {
statusCode: number,
body: unknown,
}
export class GotError extends Error {
response: GotErrorResponse;
constructor(err: RequestError) {
const statusCode = err.response?.statusCode || 0;
const { method, url } = err.options;
const body = err.response?.body || err.message;
const message = [[statusCode, method, url]
.filter(Boolean)
.join(' '), JSON.stringify(body)]
.filter(Boolean)
.join('\n');
super(message);
this.response = {
statusCode,
body,
};
}
}
/**
* Wrapper to got library
*
* @param uri - URI to request
* @param options - Got options
* @returns Request's response
*/
export const got = async (uri: string | URL, options?: OptionsOfUnknownResponseBody) => {
try {
return await originGot(uri, options);
} catch (err) {
throw new GotError(err);
}
}; |
This behavior is rather peculiar. The semantics of HTTP responses do not always align with the business logic of applications, and in some cases, a 404 status is a valid response. In the current implementation, it seems that all concise examples from the documentation need to be wrapped in a try/catch block, which is presented as a downside when comparing it to the standard const GET = async (request: NextRequest): Promise<NextResponse> => {
try {
const body = await got.get('...').json();
return NextResponse.json({
status: StatusCodes.OK,
body
});
}
catch (error: HTTPError) {
let status = error.response?.statusCode;
switch (status) {
case StatusCodes.NOT_FOUND:
let body = {};
try {
const body = JSON.parse(error.response?.body);
return NextResponse.json({status, body)});
}
catch (error) {
return NextResponse.json({status)});
}
default:
return NextResponse.json({status: StatusCodes.INTERNAL_SERVER_ERROR});
}
}
}; When I used the fetch method, I didn't encounter such issues. |
What problem are you trying to solve?
I'm using Github API. On error (non-200 http status)
got
throws an HTTPError which is by default printed like this:It gives me no information from the response body to be able to debug the issue (e.g on the local machine, or from the server logs in production).
Describe the feature
What I'd like to see is that it'd print the
response.body
, e.g by including it in Error.message (maybe limiting the maximum length, for sanity). Now I need to add this code to achieve the needed result:Then it prints like this:
And suddenly the error message becomes very useful.
I'm aware that I can use Hooks, extend my
got
instance with some error-handling hook to achieve that (that what I'm going to try now). But wouldn't everyone benefit from such feature enabled by default ingot
? Or behind a non-default configuration flag?...
Checklist
The text was updated successfully, but these errors were encountered: