Open kirillgroshkov opened 4 years ago
szmarczak
commented
4 years ago 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.
sindresorhus
commented
4 years ago Agreed
yeo-yeo
commented
4 years ago FWIW, I had the same problem as @kirillgroshkov today and would have found the proposed functionality useful and intuitive
kyler-hyuna
commented
4 years ago 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.
LeonardoRick
commented
3 years ago 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.
Hazmi35
commented
3 years ago 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.
felixfbecker
commented
3 years ago got.HTTPError has a lot of useful properties within request and response, but they are non-enumerable for some reason. This means that they don't get printed by console.error() (which uses util.inspect() under the hood). The only property that is enumerable is timings, which is probably the least useful property out of all of them. This makes the log output of Got errors very verbose, with providing almost no useful information in that output (very bad signal/noise ratio):
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, error.options is enumerable so it looks like this:
We could attach error.body but trimmed down to 100 characters.
@sindresorhus Any thoughts?
sindresorhus
commented
3 years ago 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:
process.env.NODE_ENV === 'DEBUG'.szmarczak
commented
3 years ago Make it opt-in via an option.
It's already possible via the beforeError hook.
jgehrcke
commented
3 years ago We could attach error.body but trimmed down to 100 characters.
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.
szmarczak
commented
3 years ago the security problem clearly is in the HTTP server
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.
jgehrcke
commented
3 years ago The server may output sensitive info which is useful for debugging.
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.
szmarczak
commented
3 years ago In production mode, no sensitive data (such as error detail) is supposed to leak into the response body.
Unfortunately the world isn't perfect :P
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.
I agree. But still the security concern doesn't go away. We need more feedback.
erickhavel
commented
2 years ago Hmm. What status code is my API supposed to return when the client hasn't specified enough parameters (in a POST request)? I've been using 400, which might arguably be too generic, but Got trips on both 412 (Precondition Failed, which seems like the most correct code) and 422 (Unprocessable Entity).
I don't see a fitting 2xx/3xx code for this scenario. And to be clear I don't mind the lack of descriptive error messages, I'm more worried about the code execution stopping and the user not receiving my helpful/descriptive error message asking to add the specific missing parameters.
Some discussions here:
https://stackoverflow.com/questions/16133923/400-vs-422-response-to-post-of-data
erickhavel
commented
2 years ago Is my comment irrelevant? Or am I stupid for thinking that it's fine for API's to return 4xx's intentionally, that client's shouldn't crash due to it? Or is it just due to the thread hijacking that my comment got market off-topic?
If someone else stumbles upon this same issue: Needle is the only Node request library I found that handles 4xx like you'd (or let's say I would) expect and outputs the response body instead of throwing an error.
szmarczak
commented
2 years ago @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.
aalexgabi
commented
2 years ago 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 https://github.com/sindresorhus/got/issues/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:
async setMediaStatus(mediaId, status, message) {
const url = `/media/${mediaId}/events`;
const body = {
key: 'status',
meta: {
type: status,
message,
},
};
try {
await this.api.post(url, {
json: body,
}).json();
} catch (err) {
err.method = 'POST';
err.url = url;
err.body = body;
throw err;
}
}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.
kirillgroshkov
commented
2 years ago For the record, I (the topic starter) "solved" it for myself by wrapping got with some function that adds beforeError hook by default (and alters some other defaults, unrelated to this issue), and it lives here: https://github.com/NaturalCycles/nodejs-lib/blob/master/src/got/getGot.ts#L68
@aalexgabi, your last message very well described the experience that I had when opening this issue.
szmarczak
commented
2 years ago 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.
This is on purpose. Copy n' paste those to reproduce the error on any Got instance.
throrin19
commented
1 year ago 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
AriVagelatos-KSO
commented
11 months ago 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:
Got.get(reqUrl)
.then((response: Response<string>) =>
JSON.parse(response.body)
)
.catch((error: any) => {
logger.info("I would like to output the response body here");
logger.error(error);
});
thank you!
throrin19
commented
10 months ago 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);
}
};
monolithed
commented
10 months ago 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 fetch approach.
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.
oles
commented
1 day ago Here's how I ended up with a much clearer logged error for my use case - I just wanted something I could add to the hooks and be done:
function improveLoggedGotError(error) {
// it's rarely needed to see the options used to make a request,
// and the object is huge and has a lot of unnecessary information
Object.defineProperty(error, 'options', { enumerable: false });
// unless you're debugging timings, they're just noise
Object.defineProperty(error, 'timings', { enumerable: false });
// this is often exactly what you want to see when logging an error,
// as it can contain the reason why the request failed
if (error.response) {
Object.defineProperty(error, 'response', { enumerable: true });
// I don't know why the request property is set on the response object
Object.defineProperty(error.response, 'request', { enumerable: false });
// removes some internal properties noise from the response object
Object.defineProperty(error.response, '_events', { enumerable: false });
Object.defineProperty(error.response, '_readableState', { enumerable: false });
Object.defineProperty(error.response, '_writableState', { enumerable: false });
// the rawBody is the body as a buffer, and is rarely needed when logging
Object.defineProperty(error.response, 'rawBody', { enumerable: false });
// The statusCode, statusMessage, and headers, are all getters/setters,
// they're also not writeable, so they can't be changed so their values can be logged,
// so the best option is to add a non-getter property with the wanted values
Object.defineProperty(error.response, 'nonGetterValues', {
value: {
statusCode: error.response.statusCode,
statusMessage: error.response.statusMessage,
headers: error.response.headers,
},
enumerable: true,
});
}
return error;
};Use it like so:
got.get(`https://example.org`, {
hooks: {
beforeError: [improveLoggedGotError],
},
});:v:
What problem are you trying to solve?
I'm using Github API. On error (non-200 http status)
gotthrows 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
gotinstance 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