rubyforgood / rubyforgood/Flaredown

Four API faults found while backfilling coverage, documented but unfixed

Open
#913 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

language:ruby type:bug
Dominant language
Ruby
Stars
50
Forks
21
Avg merge
6d 12h
Merged PRs (30d)
16

Description

Four pre-existing faults found while backfilling coverage in #912. Each is already pinned by a spec on that branch, asserting current behaviour with a comment explaining what correct would look like — so nothing here is silently wrong, and a future fix has to be deliberate rather than accidental. They were left unfixed because each changes an API response shape or is cosmetic, and neither belonged in a PR about coverage.

Listed roughly in order of how much they matter.


1. PostableSerializer returns raw Mongo documents, exposing encrypted_user_id

GET /api/postables sideloads posts, comments, tags, conditions, symptoms and treatments as raw Mongoid documents rather than through the Api::V1 serializers.

"posts": [{
  "_id": "6aa0653608be182a8474ee71",
  "encrypted_user_id": "abcd1234",
  "last_commented": "...", "comments_count": 1, ...
}]

Expected via Api::V1::PostSerializer would be id, type, user_name, priority, comments — and no encrypted_user_id.

Cause. PostableSerializer builds its sideloads with a bare ActiveModel::ArraySerializer:

posts: ActiveModel::ArraySerializer.new(posts, scope: current_user)

ArraySerializer#serializer_for resolves by unqualified class name — it looks for PostSerializer, while this app defines Api::V1::PostSerializer. AMS finds nothing and falls back to DefaultSerializer, which is just object.as_json. Controllers that render json: directly are unaffected, because AMS's controller integration supplies the namespace.

Consequences. The wrong key (_id not id), missing serializer-computed attributes, and encrypted_user_id handed to the client. That value is the Postgres↔Mongo join key; it is ciphertext, not a plaintext id, so this is not a direct disclosure of user ids, but it is internal plumbing that no client needs.

Fix. Pass namespace:. Correction to an earlier version of this issue: I originally wrote that this changes the payload for both clients. That was wrong about native, which does not reference postables at all. And for Ember it is a repair rather than a break — frontend/app/serializers/{post,comment}.js use ActiveModelSerializer, whose primaryKey is id, and there is no _id mapping anywhere in the frontend. Ember Data therefore cannot match the sideloaded records against the post_ids/comment_ids on the fake postable today, so the profile feed is already degraded.

Pinned by spec/controllers/api/v1/postables_controller_spec.rb, "emits sideloaded records as raw documents, not through Api::V1 serializers".


2. PatternsController#show is unreachable

Every request to it fails, one of two ways:

  • A plain GET /api/patterns/:id returns 422 Required parameter missing: pattern
  • Nesting the id where the action looks for it returns 404, even though the pattern exists
def show
  pattern = Pattern.find_by(id: pattern_params[:id])
  render json: pattern
end

def pattern_params
  params.require(:pattern).permit(:name, :start_at, :end_at, includes: [...])
end

pattern_params requires a pattern key that a normal show request does not send, and does not permit :id, so the lookup runs with nil and Mongoid raises DocumentNotFound. The action also ignores the @pattern that load_and_authorize_resource has already loaded for it.

Fix. render json: @pattern. Low risk — but it turns a 404 into a 200, so it is a behaviour change, and no client appears to call it today.

Pinned by two examples in spec/controllers/api/v1/patterns_controller_spec.rb.


3. PatternCreator silently drops the dates it is given

def initialize(options)
  @start_at = options[:start_at]
  @end_at   = options[:end_at]
  ...
end

def create
  Pattern.create(name: name, includes: includes, encrypted_user_id: encrypted_user_id)
end

start_at and end_at are read, exposed as attr_accessor, and then never passed to Pattern. A range supplied at creation is accepted and discarded, and the caller gets back a persisted pattern with both fields nil.

PatternsController#create permits both, so the API advertises them.

Fix. Either pass them through or stop accepting them. Worth checking whether any client sends them before choosing.

Pinned by spec/services/pattern_creator_spec.rb, "does not persist the start and end dates it was given".


4. Oracle refusal renders an invalid status symbol

render json: {errors: "Unauthorized"}, status: :unauthorised

:unauthorised is the British spelling and is not one of Rack's status symbols, so this raises ArgumentError instead of answering 401. In production ExceptionLogger's rescue_from "Exception" turns that into a 422 quoting the invalid symbol.

The edit is still correctly refused, which is why this has gone unnoticed — only the status code and message are wrong.

Fix. One character: :unauthorized. The most clearly safe of the four; kept here only to keep the set together.

Pinned by spec/controllers/api/v1/oracle_requests_controller_spec.rb, "refuses an edit from somebody without the token, but with the wrong status".



5. Usernameable#user_name rescues an exception class that is never raised

Found while fixing #1, and not fixed — it is unrelated to the four above.

def user_name
  Profile.select(:screen_name).find_by!(user_id: SymmetricEncryption.decrypt(encrypted_user_id)).screen_name
rescue SymmetricEncryption::CipherError
  ""
end

The rescue is meant to degrade to an empty name when encrypted_user_id will not decrypt. It never fires, because the error actually raised is OpenSSL::Cipher::CipherError, which is not a SymmetricEncryption::CipherError:

SymmetricEncryption::CipherError defined? "constant"
raised: OpenSSL::Cipher::CipherError
is a SymmetricEncryption::CipherError? false

Reachable in practice only if a stored id stops decrypting — a key rotation, or data written outside the app — so this is defensive code that does not defend. Note that find_by! is also unrescued, so a decryptable id whose profile has since been deleted raises RecordNotFound through the serializer.

Low priority, but worth either fixing the rescue or deleting it, rather than leaving something that reads as handled and is not.


Ordering note

The specs in #912 assert current behaviour. Fixing any of these will turn the corresponding example red, which is intended — the failure names the fault and points at the fix. Whoever takes one of these should update that example in the same commit.

🤖 Generated with Claude Code

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the pinned specs in spec/controllers/api/v1/postables_controller_spec.rb, spec/controllers/api/v1/patterns_controller_spec.rb, spec/services/pattern_creator_spec.rb, and spec/controllers/api/v1/oracle_requests_controller_spec.rb, then inspect the referenced serializers, controllers, and service. Choose one fault and confirm its current behavior before changing it. Done means the selected API behavior is corrected and its corresponding example is updated; review the client usage before changing response shapes or date handling.

Written by the indexing model from the issue text.

Assessment

Tech stack
mongodb, rails, ruby
Domain
api, backend, database
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.