Establish internal API standards

[mike-gorman-bitwarden]

Establish the internal API standards of a service-oriented architecture.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

Reviewed ADR-0036 and the internal-api-standards.md living standard at e3e3a75, covering all sixteen commits on the branch. The prior rounds' findings have all either landed (escaping scope and the backslash rule, response-side breaking changes, read-one and bulk-delete soft-delete semantics, the PATCH absent-field contradiction, 201 on a create-via-PUT, four added JSON:API divergences) or been answered with reasoning, so this pass worked the sections no earlier thread reached: resource identity on update, create responses, paging guarantees, and operation path shapes. Four findings, each an undefined case where two services applying the page as written would answer a caller differently; CI is green and the ADR frontmatter, anchors, and sidebar position check out.

Code Review Details

• ⚠️ : Body id/type need not match the path on PUT, and JSON:API's 409 mismatch rule covers only PATCH
  • docs/architecture/server/internal-api-standards.md:405
• ⚠️ : A create answering 204 leaves the server-generated ID undiscoverable; no Location header is required
  • docs/architecture/server/internal-api-standards.md:317
• ⚠️ : Paging requires no deterministic ordering, so paging through a collection can duplicate and skip resources
  • docs/architecture/server/internal-api-standards.md:617
• ❓ : Two path shapes for non-CRUD operations (/actions/{action} and :{operation}) with no rule for choosing
  • docs/architecture/server/internal-api-standards.md:765

[cloudflare-workers-and-pages]

Deploying contributing-docs with    Cloudflare Pages

Latest commit:	e3e3a75
Status:	✅  Deploy successful!
Preview URL:	https://813e7ca8.contributing-docs.pages.dev
Branch Preview URL:	https://docs-adr-0036-internal-api-s.contributing-docs.pages.dev

View logs

[quexten]

What happens if there is a vulnerability we need to fix and cannot fix without making a breaking change?

What happens if there is a cryptographic stability issue that we cannot fix without making a breaking change? (See recent key id tracking of vault items)?

[mike-gorman-bitwarden]

Two places in the standard cover this, and neither requires a new version.

For a vulnerability, that's a bug, and
Breaking changes and versioning
says:

Bugs, however, SHOULD be fixed "in place", without creating new versions of the API, even if the
changes would technically be considered breaking changes.

That's deliberately SHOULD and not MAY: versioning a security fix would leave the old, vulnerable
version in service, which is the opposite of what you want.

For something like the key ID tracking work, it depends on whether you'd call it a defect. If it is,
the same sentence applies. If it's better described as hardening than as fixing something broken,
the paragraph just above it covers you:

A team MAY make a breaking change in place when it can account for every caller — either because
the change has been coordinated with them, or because the rejection surfaces somewhere the caller or
the user can act on it.

Either way: fix it in place, on the existing version.

[quexten]

We have a few data points which essentially are just string encloded blobs. Examples are: vault item data bobs, encryption keys, etc. If these are changed, the json shape stays the same, but the allowed set of values in the string field changes. Would this need a newly versioned API?

[mike-gorman-bitwarden]

Yes - by the letter of the definition, narrowing the allowed values of a string field is item 4,
"adding additional constraints to a field," so it does count as a breaking change. But to answer your
question directly: no, it would not require a newly versioned API. I've updated the rule to make that clear. The definition is now "a breaking change is any change that causes a request that is valid today to be rejected tomorrow," followed by the same four-item list, and then:

The verb is SHOULD NOT rather than MUST NOT because these are internal APIs and we own every caller. A team MAY make a breaking change in place when it can account for every caller — either because the change has been coordinated with them, or because the rejection surfaces somewhere the caller or the user can act on it.

What we want to avoid is a situation where an API is changed, requests start getting rejected, and the user has no way to fix it (i.e. to get the client or UI they are using to "do the right thing"). But
if I click save one day and get an error telling me something isn't valid and needs correcting, and
the client/UI lets me fix it, that is perfectly fine.

So for the blob cases specifically: tightening what a vault item data blob or an encryption key field
will accept is a breaking change a team may make in place, provided the rejection surfaces somewhere the user or the calling service can act on it. If it would instead surface as a silent sync failure with no remediation path, that is the case the standard is trying to prevent — and then it does belong behind a new version.

[quexten]

What we want to avoid is a situation where an API is changed, requests start getting rejected, and the user has no way to fix it (i.e. to get the client or UI they are using to "do the right thing"). But
if I click save one day and get an error telling me something isn't valid and needs correcting, and
the client/UI lets me fix it, that is perfectly fine.

In this case the requests are not valid compared to our intention, but valid as in "accepted by the server today". But these already do not lead to valid data. [specifically we are adding guards to prevent certain kinds of client bugs using wrong keys resulting in undecryptable vault items, via server validation of metadata].

So i guess valid here means "as compared to our intention" / needs to not obviously break the clients for users, which our use case does not. Thanks, that is very helpful!!

So for the blob cases specifically: tightening what a vault item data blob or an encryption key field
will accept is a breaking change a team may make in place, provided the rejection surfaces somewhere the user or the calling service can act on it. If it would instead surface as a silent sync failure with no remediation path, that is the case the standard is trying to prevent — and then it does belong behind a new version.

I want to drill in on this a bit more. Suppose we add a new type of encryption today. Existing clients will not understand what this data field means. The data shape did not change, but the set of legal values when reading data expanded. I know we consider narrowing when writing a breaking change, but do we consider widening when reading a breaking change? Since existing clients would not understand this data type?

How would we deal with this? Would we have to add client support code to ignore unknown values encrypted with this vault item type?

What if the encryption keys of the users themselves change to a type the client does not support yet, or other cryptographic operations are necessary that the client doesn't support yet / if the actions they can do are available on the API level, but not permitted by the cryptographic format version of their account?

If we no longer define a set of supported client versions, I'm not sure I understand how to deal with this situation.

[mike-gorman-bitwarden]

If we no longer define a set of supported client versions, I'm not sure I understand how to deal
with this situation.

We do still define them. Nothing in these standards changes that. These standards are for internal,
service-to-service APIs only. Mobile, CLI, and browser clients don't call internal APIs, so whatever
client support policy we have today remains untouched.

Regarding "widening when reading", I don't think that one is an API versioning question at all. The
API says the field is a string. After you add a new encryption type it is still a string, it
still deserializes, and it still passes validation. What changed is the meaning of the blob. As
far as the API is concerned, it's just bytes.

So versioning the API wouldn't make a difference because the client still can't decrypt the value once
it has it.

I think it is a data format versioning question. From what I have seen, EncString carries its own
type and EncryptionType is already up to 7 (CoseEncrypt0), so we have done exactly what you are
describing at least once without needing a new API version. A client that runs into a type it doesn't
recognize knows it doesn't recognize it, because the type is right there in the data. What the client
does about that is a crypto question, not an API question.

Regarding account-level crypto versions, I don't think API versioning helps there, either. If the
account is in a format the client can't handle, the data is in that format no matter which endpoint
the client calls. The client has to look at the account's crypto version and tell the user to update
rather than try and make a mess of it.

Definitely an issue but it is more of a client capability question, I think, and less of an API
standards issue.

[quexten]

This has historically not been true, and has been solved by the +-3 versions support guarantee. If a client / server i outside of that range, we do not support that. So after three versions you are able to remove deprecated fields.

[mike-gorman-bitwarden]

You're right about the history — ±3 client support is how we have solved this, and it does let you
drop a deprecated field once the window passes. Let me be more precise about why I don't think it
carries forward.

±3 is a guarantee about clients against a platform versioned as a whole. It works because "three
versions back" is a single, fleet-wide coordinate that means the same thing everywhere. That works in
a world of regular, formal "releases" that come out every quarter, or maybe every month. But in a SOA
of independently-deployable services, each service is versioned and deployed independently. Some
slowly-changing services may run version 1.0.0 for months (or even years) while some other service
is getting bug fixes and enhancements multiple times per week — there is no shared clock against
which "three versions" means the same thing for a service that ships weekly and one that hasn't
shipped since March.

In this world, service version numbers are strictly semantic version numbers
where the patch number increments if the new release contains only bug fixes, the minor number
increments if the new release contains one or more enhancements, and the major number increments if
there are breaking changes.

Ideally, there won't be any major version numbers as we strive to avoid making breaking changes or,
if there are, we will strive to limit them. So counting the last N version numbers to determine when
it is okay to start dropping fields (or making them required) doesn't really work in this world.

Having said that, none of this precludes a team from enhancing APIs, rolling out updates to all
consumers, and then making the changes to the API that would, otherwise, be breaking. Example:

1. API adds field foo but makes it optional.
2. Every consumer is updated to start populating foo and to stop populating bar.
3. API is updated to make foo required and gets rid of field bar.

By the letter of the law, step 3 would be a "breaking change" but, in this case, the team has made
sure all consumers are populating foo and no longer populating bar.

I guess the language could be tightened up to say you can't make a field required unless you know
that all consumers are populating it but I lean toward just keeping it simple with "APIs SHOULD NOT
make breaking changes" and not try to spell out every situation where it is warranted. The
now-softened "should not" provides the general guidance and the rest is up to the team.

[quexten]

We will have lots of breaking changes from upcoming stability enhancements to our vault items alone. "Additional constraints on a field" each vault item will gain metadata validation that enforces that the correct key was used to encrypt the vault item. Would each of these require a new API version?

If so we will see a burst of changes and our current API under this lens is not yet stable enough to commit to a stable contract.

[quexten]

Or, I guess we could define this as "intended but not enforced behavior" (a bug) and then by the next paragraph it would be enforced later?

[mike-gorman-bitwarden]

Those are good points and I have done two things to try to accommodate them:

1. Soften the standard to SHOULD NOT.
2. Carve out an explicit exception for "security and cryptographic issues".

Having said that, when these scenarios come up, I try to keep in mind the target architecture is a fleet of independently-deployable services running in production with zero downtime (achieved via rolling updates or "rollouts"). If I have changes to make that are breaking changes, then I am faced with one of two options:

1. Roll out the breaking changes and accept that I am about to break some part of the system for some number of users for some period of time (until I can get corresponding changes rolled out to all clients/other services).
2. Try to coordinate the rollout so that the breaking changes and the corresponding client/consuming-service changes are all rolled out together.

Neither of these are good options.

• Obviously, I do not want to break the system. And, if I have no control over forcing all customers to upgrade to new clients (e.g. CLI, mobile, etc.) then I have no control over how long the window is where the system is broken.
• And if I get into a habit of coordinating rollouts of servers, then I am losing perhaps the single-most important characteristic of my service-oriented architecture: independently-deployable services. It is not independently-deployable if the rollout has to be coordinated with some other service or client update.

We will have lots of breaking changes from upcoming stability enhancements to our vault items alone. "Additional constraints on a field" each vault item will gain metadata validation that enforces that the correct key was used to encrypt the vault item

This may or may not amount to a breaking change. For example, sometimes a company will say "Starting on November 1, we will now start requiring all login attempts to required MFA." That is a change in behavior but it is not a breaking change. Users may now have to jump through a new hoop, but the system isn't broken. If this is the type of change you are thinking of ("each vault item will gain metadata validation"), where the new validation is handled by the client or the UI and the user is informed that the key they are using is not correct, etc., that is not a breaking change.

Or, I guess we could define this as "intended but not enforced behavior" (a bug) and then by the next paragraph it would be enforced later?

Right, it would be a bug, in which case it should be fixed "in place", even if doing so would technically be a breaking change. I've updated the language accordingly in 4acc040:

Bugs, however, SHOULD be fixed "in place", without creating new versions of the API, even if the changes would technically be considered breaking changes.

No need to single out specific classes of bug — any bug qualifies.

[quexten]

The way we have dealt with this in the past is "Roll out support for a new feature / format first, then a few "versions" later start migrating to the feature so that most clients support the feature".

[quexten]

We must make sure to not have requests that can be misinterpreted in this case. For example, suppose you have a request such as:

{
key_1: abcabcabc
key_2 (new): defdefdef
data: [encrypt(abcabcabc,hello), encrypt(defdefdef, hello)]
}

then ignoring "key_2" will now mean that the data has permanently become inaccessible. Or, I guess we need to formalize and ban this change type?

[mike-gorman-bitwarden]

These do not look like ordinary fields defined by a fixed schema. This looks like an API who, by design, is expected to loop through every key present and process it which is a different sort of API than the "usual" breed I was trying to describe. In your case, if the API failed to loop through all of the keys present, that would just be a bug.

But if this really is a "normal" API, then the scenario you are describing is this:

1. API adds a new optional field key_2.
2. Consumers get updated to start populating key_2 (along with the encrypted value).
3. Service is rolled back to a previous version that does not know anything about key_2.
4. Consumers are not rolled back; they continue sending key_2 which the service is now ignoring.

So, yes, that is exactly what would happen in that scenario — and I want to be more precise about how bad it is than I was. It isn't that key_2 fails to save. The service stores data, which contains a value encrypted under key_2, and discards key_2 itself. That element is then permanently undecryptable, which is materially worse than a field not persisting.

For an ordinary field I would still take availability over completeness: a system where one new field doesn't persist beats a system where save fails outright. But that trade only holds while the damage is recoverable. Here it isn't, and a rejected save is loud and fixable where silent unrecoverable data is neither.

On your second suggestion, I agree with the constraint. The one thing it can't be is a runtime rule: a rolled-back service doesn't know key_2 exists, and can't know that data references it, so it has no way to detect the hazard and reject the request. It has to hold at design time instead — a new optional field has to be safe to ignore, and if dropping it would orphan data the request does store, it belongs behind a new version rather than as an optional field on the existing one.

I'm going to leave that out of the standard for now rather than spell out a case this specific, on a "less is more until it's clear more is needed" basis. But I think it is the right rule, and if we see it come up more than once we should write it down.