Skip to content

wfe2: omit order expires when unset#8778

Closed
eyupcanakman wants to merge 1 commit into
letsencrypt:mainfrom
eyupcanakman:fix/wfe2-order-expires-omitempty
Closed

wfe2: omit order expires when unset#8778
eyupcanakman wants to merge 1 commit into
letsencrypt:mainfrom
eyupcanakman:fix/wfe2-order-expires-omitempty

Conversation

@eyupcanakman

Copy link
Copy Markdown

orderJSON.Expires was a bare time.Time with no omitempty, set from order.Expires.AsTime(). An order with no expiry has a zero value there, so the response serialized "expires":"1970-01-01T00:00:00Z" instead of leaving the field out. The fix switches it to a *time.Time with omitempty and sets it only when the order has an expiry, the same way IsAnyNilOrZero guards other optional fields in wfe2.

Addresses the order.expires item in #8711.

orderJSON.Expires was a bare time.Time with no omitempty, so an order without an expiry serialized expires as a placeholder timestamp instead of omitting it. Make it a *time.Time with omitempty and set it only when present.

Addresses letsencrypt#8711.
@eyupcanakman eyupcanakman requested a review from a team as a code owner June 4, 2026 12:26
@eyupcanakman eyupcanakman requested a review from jsha June 4, 2026 12:26
@jsha

jsha commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Hi @eyupcanakman ! Welcome to Boulder, and thanks for the contribution.

As noted in #8711, in Boulder an order expires is always set. I'd rather not add code to handle the case where it's unset, for a couple of reasons:

  • We wouldn't have any test coverage of that case.
  • It would imply to the reader that the field was sometimes unset.

What we really want is a check that Expires is never zero, erroring if so. We already have that in the places in wfe.go where we receive an error. So I consider that item under #8711 invalid. I'll comment to clarify. Sorry you spent your time on an issue that turned out to be invalid!

@jsha jsha closed this Jun 10, 2026
@jsha jsha mentioned this pull request Jun 10, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants