From 1c407a28cd427dbab1642707bfae3e08011462da Mon Sep 17 00:00:00 2001 From: Nick Rolfe Date: Thu, 24 Nov 2022 14:00:05 +0000 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Harry Maclean --- .../queries/security/cwe-209/StackTraceExposure.qhelp | 2 +- .../security/cwe-209/examples/StackTraceExposure.rb | 6 +++--- .../security/cwe-209/StackTraceExposure.expected | 10 +++++----- .../query-tests/security/cwe-209/StackTraceExposure.rb | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/ruby/ql/src/queries/security/cwe-209/StackTraceExposure.qhelp b/ruby/ql/src/queries/security/cwe-209/StackTraceExposure.qhelp index 916f6cfcbeb..793138256ff 100644 --- a/ruby/ql/src/queries/security/cwe-209/StackTraceExposure.qhelp +++ b/ruby/ql/src/queries/security/cwe-209/StackTraceExposure.qhelp @@ -33,7 +33,7 @@ Either suppress the stack trace entirely, or log it only on the server.

In the following example, an exception is handled in two different ways. In the -first version, labeled BAD, the exception is exposted to the remote user by +first version, labeled BAD, the exception is exposed to the remote user by rendering it as an HTTP response. As such, the user is able to see a detailed stack trace, which may contain sensitive information. In the second version, the error message is logged only on the server, and a generic error message is diff --git a/ruby/ql/src/queries/security/cwe-209/examples/StackTraceExposure.rb b/ruby/ql/src/queries/security/cwe-209/examples/StackTraceExposure.rb index 591be56014f..86cd38f5c30 100644 --- a/ruby/ql/src/queries/security/cwe-209/examples/StackTraceExposure.rb +++ b/ruby/ql/src/queries/security/cwe-209/examples/StackTraceExposure.rb @@ -4,15 +4,15 @@ class UsersController < ApplicationController do_computation() rescue => e # BAD - render e.backtrace, content_type: "text/plain" + render body: e.backtrace, content_type: "text/plain" end def update_good(id) do_computation() rescue => e # GOOD - log e.backtrace - redner "Computation failed", content_type: "text/plain" + logger.error e.backtrace + render body: "Computation failed", content_type: "text/plain" end end \ No newline at end of file diff --git a/ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.expected b/ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.expected index 7a0e35973be..da66e5e4dc1 100644 --- a/ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.expected +++ b/ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.expected @@ -1,10 +1,10 @@ edges -| StackTraceExposure.rb:11:10:11:17 | call to caller : | StackTraceExposure.rb:12:12:12:13 | bt | +| StackTraceExposure.rb:11:10:11:17 | call to caller : | StackTraceExposure.rb:12:18:12:19 | bt | nodes -| StackTraceExposure.rb:6:12:6:22 | call to backtrace | semmle.label | call to backtrace | +| StackTraceExposure.rb:6:18:6:28 | call to backtrace | semmle.label | call to backtrace | | StackTraceExposure.rb:11:10:11:17 | call to caller : | semmle.label | call to caller : | -| StackTraceExposure.rb:12:12:12:13 | bt | semmle.label | bt | +| StackTraceExposure.rb:12:18:12:19 | bt | semmle.label | bt | subpaths #select -| StackTraceExposure.rb:6:12:6:22 | call to backtrace | StackTraceExposure.rb:6:12:6:22 | call to backtrace | StackTraceExposure.rb:6:12:6:22 | call to backtrace | $@ can be exposed to an external user. | StackTraceExposure.rb:6:12:6:22 | call to backtrace | Error information | -| StackTraceExposure.rb:12:12:12:13 | bt | StackTraceExposure.rb:11:10:11:17 | call to caller : | StackTraceExposure.rb:12:12:12:13 | bt | $@ can be exposed to an external user. | StackTraceExposure.rb:11:10:11:17 | call to caller | Error information | +| StackTraceExposure.rb:6:18:6:28 | call to backtrace | StackTraceExposure.rb:6:18:6:28 | call to backtrace | StackTraceExposure.rb:6:18:6:28 | call to backtrace | $@ can be exposed to an external user. | StackTraceExposure.rb:6:18:6:28 | call to backtrace | Error information | +| StackTraceExposure.rb:12:18:12:19 | bt | StackTraceExposure.rb:11:10:11:17 | call to caller : | StackTraceExposure.rb:12:18:12:19 | bt | $@ can be exposed to an external user. | StackTraceExposure.rb:11:10:11:17 | call to caller | Error information | diff --git a/ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.rb b/ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.rb index a9faa66af02..6091984df1a 100644 --- a/ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.rb +++ b/ruby/ql/test/query-tests/security/cwe-209/StackTraceExposure.rb @@ -3,13 +3,13 @@ class FooController < ApplicationController def show something_that_might_fail() rescue => e - render e.backtrace, content_type: "text/plain" + render body: e.backtrace, content_type: "text/plain" end def show2 bt = caller() - render bt, content_type: "text/plain" + render body: bt, content_type: "text/plain" end end