-
Notifications
You must be signed in to change notification settings - Fork 87
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
feat: disable policy feature when 403 or error when fetching and parsing bucket policy #902
Conversation
…ing bucket policy
if (error instanceof S3ServiceException) { | ||
switch (error.$metadata?.httpStatusCode) { | ||
case 404: | ||
console.info( | ||
"Bucket policy does not exist (404), it's ok." | ||
); | ||
return { | ||
isBucketPolicyAvailable: true, | ||
bucketPolicy: undefined, | ||
allowedPrefix: [] | ||
}; | ||
case 403: | ||
console.info("Access denied to bucket policy (403)."); | ||
return { | ||
isBucketPolicyAvailable: false, | ||
bucketPolicy: undefined, | ||
allowedPrefix: [] | ||
}; | ||
default: | ||
console.error("An S3 error occurred:", error.message); | ||
return { | ||
isBucketPolicyAvailable: false, | ||
bucketPolicy: undefined, | ||
allowedPrefix: [] | ||
}; | ||
} | ||
} | ||
|
||
if (sendResp.Policy === undefined) { | ||
return undefined; | ||
console.error( | ||
"An unexpected error occurred when fetching bucket policy", | ||
error | ||
); | ||
return { | ||
isBucketPolicyAvailable: false, | ||
bucketPolicy: undefined, | ||
allowedPrefix: [] | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather see:
try{
}catch(error){
if( !(error instanceof S3ServiceException)){
throw error;
}
switch(...
}
The reasoning is that we should be consistent with our error management policies.
If we wheren't handling S3ServiceException
we would let the exception stop the execution.
As of now, any other type of exception that might be thrown here is unexpected and we should adress it.
return { | ||
isBucketPolicyAvailable: true, | ||
bucketPolicy: parsedPolicy, | ||
allowedPrefix | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here again I'd rather have a finer handling of the exceptions.
let parsedPolicy;
try{
parsedPolicy = s3BucketPolicySchema.parse(JSON.parse(sendResp.Policy));
}catch(error){
console.error("failed to parse policy", String(error))
return {
isBucketPolicyAvailable: false,
bucketPolicy: undefined,
allowedPrefix: []
};
}
let allowedPrefix;
try{
allowedPrefix = ...
}catch(error){
console.error("Failed to extract allowed prefix from policy", String(error));
return { ... };
}
return {
isBucketPolicyAvailable: true,
bucketPolicy: parsedPolicy,
allowedPrefix
};
|
No description provided.