Skip to content

feat: ignore secure config #34

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Oct 13, 2024
Merged

feat: ignore secure config #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
960646d
feat: ignore secure config
atian25 Dec 6, 2021
463d4e4
Merge branch 'master' into config-ignore
fengmk2 Oct 13, 2024
b5c2778
Update error_view.js
fengmk2 Oct 13, 2024
5490c2a
Update error_view.js
fengmk2 Oct 13, 2024
42be239
Update error_view.js
fengmk2 Oct 13, 2024
19dadaa
Update package.json
fengmk2 Oct 13, 2024
a2cfdd3
f
fengmk2 Oct 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/error_view.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,9 +262,13 @@ class ErrorView {
* @return {Object} egg app info
*/
serializeAppInfo() {
let config = this.app.config;
if (typeof this.app.dumpConfigToObject === 'function') {
config = this.app.dumpConfigToObject().config.config;
}
return {
baseDir: this.app.config.baseDir,
config: util.inspect(this.app.config),
config: util.inspect(config),
Comment on lines +265 to +271
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Verify the configuration retrieval logic and consider error handling.

The changes to the serializeAppInfo method introduce a new way of retrieving the app configuration, which is good for flexibility. However, there are a couple of points to consider:

  1. The nested config.config access in config = this.app.dumpConfigToObject().config.config; seems unusual. Is this intentional, or should it be just config = this.app.dumpConfigToObject().config;?

  2. There's no error handling if this.app.dumpConfigToObject() throws an error. Consider adding a try-catch block to handle potential errors gracefully.

Here's a suggested improvement:

serializeAppInfo() {
  let config;
  try {
    if (typeof this.app.dumpConfigToObject === 'function') {
      const dumpedConfig = this.app.dumpConfigToObject();
      config = dumpedConfig.config || dumpedConfig;
    } else {
      config = this.app.config;
    }
  } catch (error) {
    console.error('Error retrieving app configuration:', error);
    config = this.app.config;
  }

  return {
    baseDir: this.app.config.baseDir,
    config: util.inspect(config),
  };
}

This suggestion adds error handling and provides a fallback in case of errors. It also allows for flexibility in the structure of the dumped config object.

};
}
}
Expand Down
13 changes: 12 additions & 1 deletion test/onerror.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,16 @@ describe('test/onerror.test.js', () => {
.expect(400);
});

it('should ignore secure config on html response', () => {
return app.httpRequest()
.post('/test?status=400')
.send({ test: 1 })
.set('Content-Type', 'application/json')
.expect(/keys: '<String len: 7/)
.expect('Content-Type', 'text/html; charset=utf-8')
.expect(400);
});

it('should return parsing json error on json response', () => {
return app.httpRequest()
.post('/test?status=400')
Expand Down Expand Up @@ -287,7 +297,8 @@ describe('test/onerror.test.js', () => {
await app.close();

const warnLog = path.join(__dirname, 'fixtures/onerror-4xx/logs/onerror-4xx/onerror-4xx-web.log');
assert(/POST \/body_parser] nodejs\..*?Error: request entity too large/.test(fs.readFileSync(warnLog, 'utf8')));
const content = fs.readFileSync(warnLog, 'utf8');
assert.match(content, /POST \/body_parser] nodejs\..*?Error: request entity too large/);
});
}

Expand Down
Loading