Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
16 changes: 12 additions & 4 deletions internal/restapi/schedule_for_stop_handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,16 @@ func (api *RestAPI) scheduleForStopHandler(w http.ResponseWriter, r *http.Reques
// Get the date parameter or use current date
dateParam := r.URL.Query().Get("date")

// An unparseable date is a field error even when the ID resolves to nothing, so it has
// to be caught before the agency lookup below. Resolving the date to a service date
// needs that agency's timezone, so the parse itself stays where it is.
if dateParam != "" {
if err := utils.ValidateServiceDate(dateParam); err != nil {
api.validationErrorResponse(w, r, map[string][]string{"date": {err.Error()}})
return
}
}

agency, err := api.GtfsManager.GtfsDB.Queries.GetAgency(ctx, agencyID)
if err != nil {
api.sendNotFound(w, r)
Expand All @@ -46,12 +56,10 @@ func (api *RestAPI) scheduleForStopHandler(w http.ResponseWriter, r *http.Reques

if dateParam != "" {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This comment ("only fails on an unusable agency timezone") is inaccurate, and the branch below it looks unreachable now. ValidateServiceDate (line 36) already runs ParseDate(dateParam, time.UTC) and returns 400 on any format/bounds failure; neither of ParseDate's two success paths depends on the loc argument for success/failure, so once ValidateServiceDate has succeeded, ParseDate(dateParam, loc) here can't fail for any valid loc. A bad agency timezone is also already caught earlier by loadAgencyLocation (lines 48-52), before this second parse runs. Since this branch appears dead, could it be removed (or the comment corrected if there's a case I'm missing)?

var err error
// The format was validated above, so this only fails on an unusable agency timezone.
startOfDay, err = utils.ParseDate(dateParam, loc)
if err != nil {
fieldErrors := map[string][]string{
"date": {err.Error()},
}
api.validationErrorResponse(w, r, fieldErrors)
api.serverErrorResponse(w, r, err)
return
}

Expand Down
62 changes: 62 additions & 0 deletions internal/restapi/schedule_for_stop_handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,68 @@ func TestScheduleForStopHandlerInvalidDateFormat(t *testing.T) {
}
}

func TestScheduleForStopHandlerDateValidationPrecedesLookup(t *testing.T) {
api := createTestApi(t)
defer api.Shutdown()

knownStopID := utils.FormCombinedID(mustGetAgencies(t, api)[0].ID, mustGetStop(t, api).ID)

tests := []struct {
name string
stopID string
date string
expectedStatus int
expectFieldError bool
}{
{
name: "unknown agency with an invalid date",
stopID: "99_1001",
date: "garbage",
expectedStatus: http.StatusBadRequest,
expectFieldError: true,
},
{
name: "known agency with an invalid date",
stopID: knownStopID,
date: "garbage",
expectedStatus: http.StatusBadRequest,
expectFieldError: true,
},
{
name: "unknown agency with a valid date",
stopID: "99_1001",
date: "2025-06-12",
expectedStatus: http.StatusNotFound,
},
{
name: "unknown stop with a valid date",
stopID: "25_9999999",
date: "2025-06-12",
expectedStatus: http.StatusNotFound,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
endpoint := "/api/where/schedule-for-stop/" + tt.stopID + ".json?key=org.onebusaway.iphone&date=" + tt.date
resp, model := serveApiAndRetrieveEndpoint(t, api, endpoint)

assert.Equal(t, tt.expectedStatus, resp.StatusCode)
assert.Equal(t, tt.expectedStatus, model.Code)

if !tt.expectFieldError {
return
}

data, ok := model.Data.(map[string]any)
require.True(t, ok)
fieldErrors, ok := data["fieldErrors"].(map[string]any)
require.True(t, ok)
assert.NotEmpty(t, fieldErrors["date"])
})
}
}

func TestScheduleForStopHandlerScheduleContent(t *testing.T) {
api := createTestApi(t)
defer api.Shutdown()
Expand Down
8 changes: 8 additions & 0 deletions internal/utils/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,14 @@ func ValidateDate(date string) error {
return nil
}

// ValidateServiceDate reports whether a service date parameter is parseable, in either
// of the forms ParseDate accepts. Handlers use it to reject a malformed date before
// looking up the agency whose timezone ParseDate then resolves the date against.
func ValidateServiceDate(date string) error {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

ValidateServiceDate duplicates the pre-existing ValidateDate (line 95) instead of extending or replacing it. ValidateDate (YYYY-MM-DD only) appears unused anywhere in production code, so the file now carries two similarly-named date validators with overlapping scope — a future caller or bugfix can easily reach for the wrong one. Worth considering whether ValidateDate should be removed or merged into this new function instead of living alongside it.

_, err := ParseDate(date, time.UTC)
return err
}

// SanitizeInput removes HTML tags and other potentially dangerous content
func SanitizeInput(input string) string {
// Remove HTML tags
Expand Down