Skip to content

Commit

Permalink
Fix CodeQL identified issues (#12199)
Browse files Browse the repository at this point in the history
This addresses two classes of issues that CodeQL found. One is a not as
strict as needed regexp for a domain match, where there's no literal `.`
being matched.

The other is a user controlled XSS since we run the template with
arbitrary user input. Instead we validate if the throttler with that
name actually exists before trying to execute the template.

On error we also don't reflect the input to avoid the same XSS.

Signed-off-by: Dirkjan Bussink <d.bussink@gmail.com>
  • Loading branch information
dbussink committed Feb 6, 2023
1 parent b75f13d commit 79715f3
Show file tree
Hide file tree
Showing 4 changed files with 18 additions and 7 deletions.
10 changes: 8 additions & 2 deletions go/vt/throttler/throttlerlogz.go
Expand Up @@ -24,6 +24,8 @@ import (
"strings"
"time"

"golang.org/x/exp/slices"

"vitess.io/vitess/go/vt/logz"
)

Expand Down Expand Up @@ -109,8 +111,7 @@ func throttlerlogzHandler(w http.ResponseWriter, r *http.Request, m *managerImpl
parts := strings.SplitN(r.URL.Path, "/", 3)

if len(parts) != 3 {
errMsg := fmt.Sprintf("invalid /throttlerlogz path: %q expected paths: /throttlerlogz/ or /throttlerlogz/<throttler name>", r.URL.Path)
http.Error(w, errMsg, http.StatusInternalServerError)
http.Error(w, "invalid /throttlerlogz path", http.StatusNotFound)
return
}

Expand All @@ -121,6 +122,11 @@ func throttlerlogzHandler(w http.ResponseWriter, r *http.Request, m *managerImpl
return
}

if !slices.Contains(m.Throttlers(), name) {
http.Error(w, "throttler not found", http.StatusNotFound)
return
}

showThrottlerLog(w, m, name)
}

Expand Down
2 changes: 1 addition & 1 deletion go/vt/throttler/throttlerlogz_test.go
Expand Up @@ -41,7 +41,7 @@ func TestThrottlerlogzHandler_NonExistantThrottler(t *testing.T) {

throttlerlogzHandler(response, request, newManager())

if got, want := response.Body.String(), `throttler: t1 does not exist`; !strings.Contains(got, want) {
if got, want := response.Body.String(), `throttler not found`; !strings.Contains(got, want) {
t.Fatalf("/throttlerlogz page for non-existent t1 should not succeed. got = %v, want = %v", got, want)
}
}
Expand Down
11 changes: 8 additions & 3 deletions go/vt/throttler/throttlerz.go
Expand Up @@ -17,11 +17,12 @@ limitations under the License.
package throttler

import (
"fmt"
"html/template"
"net/http"
"strings"

"golang.org/x/exp/slices"

"vitess.io/vitess/go/vt/log"
)

Expand Down Expand Up @@ -59,8 +60,7 @@ func throttlerzHandler(w http.ResponseWriter, r *http.Request, m *managerImpl) {
parts := strings.SplitN(r.URL.Path, "/", 3)

if len(parts) != 3 {
errMsg := fmt.Sprintf("invalid /throttlerz path: %q expected paths: /throttlerz or /throttlerz/<throttler name>", r.URL.Path)
http.Error(w, errMsg, http.StatusInternalServerError)
http.Error(w, "invalid /throttlerz path", http.StatusNotFound)
return
}

Expand All @@ -70,6 +70,11 @@ func throttlerzHandler(w http.ResponseWriter, r *http.Request, m *managerImpl) {
return
}

if !slices.Contains(m.Throttlers(), name) {
http.Error(w, "throttler not found", http.StatusNotFound)
return
}

showThrottlerDetails(w, name)
}

Expand Down
2 changes: 1 addition & 1 deletion go/vt/vtctl/vtctldclient/codegen/main.go
Expand Up @@ -332,7 +332,7 @@ func extractSourceInterface(pkg *packages.Package, name string) (*types.Interfac
return nil, fmt.Errorf("symbol %s was not an interface but %T", name, obj.Type())
}

var vitessProtoRegexp = regexp.MustCompile(`^vitess.io.*/proto/.*`)
var vitessProtoRegexp = regexp.MustCompile(`^vitess\.io.*/proto/.*`)

func rewriteProtoImports(pkg *types.Package) string {
if vitessProtoRegexp.MatchString(pkg.Path()) {
Expand Down

0 comments on commit 79715f3

Please sign in to comment.