Skip to content

Commit

Permalink
httputil: remove error details
Browse files Browse the repository at this point in the history
  • Loading branch information
calebdoxsey committed Oct 24, 2022
1 parent 9e23775 commit deafbd9
Show file tree
Hide file tree
Showing 9 changed files with 33 additions and 33 deletions.
2 changes: 1 addition & 1 deletion authenticate/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ func TestAuthenticate_SignOut(t *testing.T) {
identity.MockProvider{LogOutResponse: (*uriParseHelper("https://microsoft.com"))},
&mstore.Store{Encrypted: true, Session: &sessions.State{}},
http.StatusOK,
"{\"Status\":200,\"Error\":\"OK: user logged out\"}\n",
"{\"Status\":200}\n",
},
}
for _, tt := range tests {
Expand Down
11 changes: 8 additions & 3 deletions internal/httputil/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"net/http"
"net/url"

"github.com/pomerium/pomerium/internal/log"
"github.com/pomerium/pomerium/internal/telemetry/requestid"
"github.com/pomerium/pomerium/pkg/contextutil"
"github.com/pomerium/pomerium/ui"
Expand Down Expand Up @@ -52,7 +53,6 @@ func (e *HTTPError) ErrorResponse(ctx context.Context, w http.ResponseWriter, r
}
response := struct {
Status int
Error string
StatusText string `json:"-"`
RequestID string `json:",omitempty"`
CanDebug bool `json:"-"`
Expand All @@ -61,7 +61,6 @@ func (e *HTTPError) ErrorResponse(ctx context.Context, w http.ResponseWriter, r
}{
Status: e.Status,
StatusText: StatusText(e.Status),
Error: e.Error(),
RequestID: reqID,
CanDebug: e.Status/100 == 4 && (e.DebugURL != nil || reqID != ""),
DebugURL: e.DebugURL,
Expand All @@ -70,14 +69,20 @@ func (e *HTTPError) ErrorResponse(ctx context.Context, w http.ResponseWriter, r
// indicate to clients that the error originates from Pomerium, not the app
w.Header().Set(HeaderPomeriumResponse, "true")

log.Error(ctx).
Err(e.Err).
Int("status", e.Status).
Str("status-text", StatusText(e.Status)).
Str("request-id", reqID).
Msg("httputil: error")

if r.Header.Get("Accept") == "application/json" {
RenderJSON(w, e.Status, response)
return
}

m := map[string]any{
"canDebug": response.CanDebug,
"error": response.Error,
"requestId": response.RequestID,
"status": response.Status,
"statusText": response.StatusText,
Expand Down
2 changes: 1 addition & 1 deletion internal/httputil/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func TestHTTPError_ErrorResponse(t *testing.T) {
wantStatus int
wantBody string
}{
{"404 json", http.StatusNotFound, errors.New("route not known"), "application/json", http.StatusNotFound, "{\"Status\":404,\"Error\":\"Not Found: route not known\"}\n"},
{"404 json", http.StatusNotFound, errors.New("route not known"), "application/json", http.StatusNotFound, "{\"Status\":404}\n"},
{"404 html", http.StatusNotFound, errors.New("route not known"), "", http.StatusNotFound, ""},
}
for _, tt := range tests {
Expand Down
4 changes: 2 additions & 2 deletions internal/httputil/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@ func TestHandlerFunc_ServeHTTP(t *testing.T) {
f HandlerFunc
wantBody string
}{
{"good http error", func(w http.ResponseWriter, r *http.Request) error { return NewError(404, errors.New("404")) }, "{\"Status\":404,\"Error\":\"Not Found: 404\"}\n"},
{"good std error", func(w http.ResponseWriter, r *http.Request) error { return errors.New("404") }, "{\"Status\":500,\"Error\":\"Internal Server Error: 404\"}\n"},
{"good http error", func(w http.ResponseWriter, r *http.Request) error { return NewError(404, errors.New("404")) }, "{\"Status\":404}\n"},
{"good std error", func(w http.ResponseWriter, r *http.Request) error { return errors.New("404") }, "{\"Status\":500}\n"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion internal/middleware/middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func TestValidateSignature(t *testing.T) {
wantBody string
}{
{"good", []byte("secret"), []byte("secret"), http.StatusOK, http.StatusText(http.StatusOK)},
{"secret mistmatch", []byte("secret"), []byte("hunter42"), http.StatusBadRequest, "{\"Status\":400,\"Error\":\"Bad Request: internal/urlutil: hmac failed\"}\n"},
{"secret mistmatch", []byte("secret"), []byte("hunter42"), http.StatusBadRequest, "{\"Status\":400}\n"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Expand Down
6 changes: 3 additions & 3 deletions proxy/forward_auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,9 @@ func TestProxy_ForwardAuth(t *testing.T) {
wantBody string
}{
{"good verify only, no redirect", opts, nil, http.MethodGet, nil, nil, "https://some.domain.example/verify", "https://some.domain.example", &mock.Encoder{}, &mstore.Store{Session: &sessions.State{}}, allowClient, http.StatusOK, ""},
{"bad empty domain uri", opts, nil, http.MethodGet, nil, map[string]string{"uri": ""}, "https://some.domain.example/", "", &mock.Encoder{}, &mstore.Store{Session: &sessions.State{}}, allowClient, http.StatusBadRequest, "{\"Status\":400,\"Error\":\"Bad Request: https: url does contain a valid hostname\"}\n"},
{"bad naked domain uri", opts, nil, http.MethodGet, nil, nil, "https://some.domain.example/", "a.naked.domain", &mock.Encoder{}, &mstore.Store{Session: &sessions.State{}}, allowClient, http.StatusBadRequest, "{\"Status\":400,\"Error\":\"Bad Request: a.naked.domain url does contain a valid scheme\"}\n"},
{"bad empty verification uri", opts, nil, http.MethodGet, nil, nil, "https://some.domain.example/", " ", &mock.Encoder{}, &mstore.Store{Session: &sessions.State{}}, allowClient, http.StatusBadRequest, "{\"Status\":400,\"Error\":\"Bad Request: %20 url does contain a valid scheme\"}\n"},
{"bad empty domain uri", opts, nil, http.MethodGet, nil, map[string]string{"uri": ""}, "https://some.domain.example/", "", &mock.Encoder{}, &mstore.Store{Session: &sessions.State{}}, allowClient, http.StatusBadRequest, "{\"Status\":400}\n"},
{"bad naked domain uri", opts, nil, http.MethodGet, nil, nil, "https://some.domain.example/", "a.naked.domain", &mock.Encoder{}, &mstore.Store{Session: &sessions.State{}}, allowClient, http.StatusBadRequest, "{\"Status\":400}\n"},
{"bad empty verification uri", opts, nil, http.MethodGet, nil, nil, "https://some.domain.example/", " ", &mock.Encoder{}, &mstore.Store{Session: &sessions.State{}}, allowClient, http.StatusBadRequest, "{\"Status\":400}\n"},
// traefik
{"good traefik callback", opts, nil, http.MethodGet, map[string]string{httputil.HeaderForwardedURI: "https://some.domain.example?" + urlutil.QuerySessionEncrypted + "=" + goodEncryptionString}, nil, "https://some.domain.example/", "https://some.domain.example", &mock.Encoder{}, &mstore.Store{Session: &sessions.State{}}, allowClient, http.StatusFound, ""},
{"bad traefik callback bad session", opts, nil, http.MethodGet, map[string]string{httputil.HeaderForwardedURI: "https://some.domain.example?" + urlutil.QuerySessionEncrypted + "=" + goodEncryptionString + "garbage"}, nil, "https://some.domain.example/", "https://some.domain.example", &mock.Encoder{}, &mstore.Store{Session: &sessions.State{}}, allowClient, http.StatusBadRequest, ""},
Expand Down
4 changes: 2 additions & 2 deletions proxy/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,14 +331,14 @@ func TestProxy_ProgrammaticLogin(t *testing.T) {
opts, http.MethodGet, "https", "corp.example.example", "/.pomerium/api/v1/login", nil,
map[string]string{urlutil.QueryRedirectURI: "localhost"},
http.StatusBadRequest,
"{\"Status\":400,\"Error\":\"Bad Request: localhost url does contain a valid scheme\"}\n",
"{\"Status\":400}\n",
},
{
"bad redirect_uri not whitelisted",
opts, http.MethodGet, "https", "corp.example.example", "/.pomerium/api/v1/login", nil,
map[string]string{urlutil.QueryRedirectURI: "https://example.com"},
http.StatusBadRequest,
"{\"Status\":400,\"Error\":\"Bad Request: invalid redirect uri\"}\n",
"{\"Status\":400}\n",
},
{
"bad http method",
Expand Down
34 changes: 15 additions & 19 deletions ui/src/components/ErrorPage.tsx
Original file line number Diff line number Diff line change
@@ -1,30 +1,31 @@
import {ErrorPageData, PolicyEvaluationTrace} from "../types";
import SectionFooter from "./SectionFooter";
import { ListItemProps, TableCell } from "@mui/material";
import Alert from "@mui/material/Alert";
import AlertTitle from "@mui/material/AlertTitle";
import Box from "@mui/material/Box";
import Container from "@mui/material/Container";
import Paper from "@mui/material/Paper";
import Stack from "@mui/material/Stack";
import Typography from "@mui/material/Typography";
import React, { FC } from "react";
import Markdown from "markdown-to-jsx";
import {ListItemProps, TableCell} from "@mui/material";
import {CheckCircle, MinusCircle, XCircle} from "react-feather";
import Table from "@mui/material/Table";
import TableRow from "@mui/material/TableRow";
import TableHead from "@mui/material/TableHead";
import TableRow from "@mui/material/TableRow";
import Typography from "@mui/material/Typography";
import Markdown from "markdown-to-jsx";
import React, { FC } from "react";
import { CheckCircle, MinusCircle, XCircle } from "react-feather";

import { ErrorPageData, PolicyEvaluationTrace } from "../types";
import SectionFooter from "./SectionFooter";

type PolicyEvaluationTraceDetailsProps = {
trace: PolicyEvaluationTrace;
} & ListItemProps;
const PolicyEvaluationTraceDetails: FC<PolicyEvaluationTraceDetailsProps> = ({
trace,
...props
trace,
...props
}) => {
return (
<TableRow>
<TableCell align={'center'}>
<TableCell align={"center"}>
{trace.deny ? (
<XCircle color="red" />
) : trace.allow ? (
Expand All @@ -34,9 +35,7 @@ const PolicyEvaluationTraceDetails: FC<PolicyEvaluationTraceDetailsProps> = ({
)}
</TableCell>
<TableCell>
<Markdown>
{trace.explanation || trace.id}
</Markdown>
<Markdown>{trace.explanation || trace.id}</Markdown>
</TableCell>
<TableCell>
<Markdown>
Expand All @@ -63,14 +62,11 @@ export const ErrorPage: FC<ErrorPageProps> = ({ data }) => {
{data?.status || 500}{" "}
{data?.statusText || "Internal Server Error"}
</AlertTitle>
{data?.error || "Internal Server Error"}
</Alert>
</Box>
{!!data?.errorMessageFirstParagraph && (
<Box sx={{p: 4}}>
<Markdown>
{data.errorMessageFirstParagraph}
</Markdown>
<Box sx={{ p: 4 }}>
<Markdown>{data.errorMessageFirstParagraph}</Markdown>
</Box>
)}
{traces?.length > 0 && (
Expand Down
1 change: 0 additions & 1 deletion ui/src/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,6 @@ export type ErrorPageData = BasePageData & {

canDebug?: boolean;
debugUrl?: string;
error?: string;
requestId?: string;
status?: number;
statusText?: string;
Expand Down

0 comments on commit deafbd9

Please sign in to comment.